separate generator behavior into phases #2274
Conversation
There was a problem hiding this comment.
Looks like a clean port. Thanks for picking up where I left off @debs-sifive!
High level comments
I expect the PR can go further with the stage/phase conversion (e.g., not using any Driver methods), but I like this as a first step.
The dependencies generally look fine. I do tend to include all prerequisites. E.g., GenerateFirrtl has only Elaborate as a prerequisite. However, it is doing blind .gets on a view[RocketChipOptions]. Users may use GenerateFirrtl in a separate PhaseManager and that phase can throw None.get exceptions if Checks hasn't run. It makes the dependency specification more verbose, but should make them bulletproof for whatever arbitrary composition users want to do.
Some questions
- Does this PR want to keep the existing Rocket approach of separating the config package from the config name and the top-level package from the top-level name?
- I think the PR's approach of just hard-coding the default rocket test suites makes sense. However, have you thought about how to wrap that up in a
RocketTestSuiteAnnotation(tests: Seq[RocketTestSuite])?
Answers to @debs-sifive Questions
code cleanup — should I strip out the old generator entirely?
Rocket Chip doesn't have the same stringent requirements on backwards compatibility as Chisel/FIRRTL. Still, I'd try to notify stakeholders and definitely consider doing this in a subsequent PR.
extending ChiselStage vs calling ChiselStage?
I'd tend towards calling it vs. extending it (or defining at new PhaseManager).
solicit outside feedback — is it ok to change the generator command line?
Good thought. I think the Generator API is more problematic than the command line as I anticipate most Rocket Chip users are just using the makefiles directly. Rocket Chip was never published as a fat jar (though it could and probably should be).
|
Thanks for all the feedback, @seldridge!
I was trying to adhere to the old CLI as closely as possible, but it seems the consensus is that we can scrap the old CLI. Given this, I think the way you created class instead of string annotations in #2234 is the cleanest way of doing things, so I will try to go back to that.
They were hardcoded in the old generator, and I just copy/pasted the function 😬 . I will try out your suggestion. There's actually a bunch of stuff from the generator/generator utils that I copied into different files to avoid dependency issues, and I will start to clean them up... First step is probably removing the old generator entirely, since that has been okayed in ucb-bar/chipyard#424. |
debs-sifive
force-pushed
the
staged-generator
branch
from
5c11e3a to
6f9f3f8
Compare
|
@seldridge not sure why I can't request review from you, but would you mind taking another glance? |
|
Sure @debs-sifive. (I'm not a reviewer for rocket-chip, so I can't be requested.) Can provide feedback, though. |
@seldridge sorry to keep bothering you, but I am not sure of how to go about this cleanly. I initially tried separating out the test generation into another phase that created Do you have an idea of what the ideal
|
|
External file seems like a bad API. I think what we want here is two custom phases that are dynamically added to the list of targets. So, there's a default Rocket Chip Stage and then if you're doing a normal build, you add the |
|
@seldridge I think a better API would be to write something like an aspect which introspects on the design and generates the test collateral (and presumably consuming additional annotations for customization). However, IMO its out of scope of this PR so I'm personally happy as is. What do you think? |
|
Build failed with latest: maybe forget to bump firrtl into master? |
|
You'll need to wait for the actual Chisel3/FIRRTL bumps. There are API changes to the Dependency API that will land in 3.3/1.3 that will need to migrate into Rocket Chip. These changes are merged in Chisel3/FIRRTL master currently, but not into rocket as there are other accumulated changes in Chisel3/FIRRTL that need to dealt with in rocket. Sorry for the delay, but all the dependency stuff is almost there and almost fully load-bearing. |
|
yes, these commits are current chisel/firrtl master branch, but still failed to compile, I'm not sure which commits are required by this change |
This reverts commit 9d99750.
This reverts commit eadc4ec.
Revert staged generator PRs (#2329) * Revert "updating README examples with Make changes (#2322)" This reverts commit 875dcd3. * Revert "fixing aspect generation (#2309)" This reverts commit bbeb257. * Revert "separate generator behavior into phases (#2274)" This reverts commit 9d99750. (cherry picked from commit e170f8f)
Excuse me, did these questions sloved? |
|
Master of rocket is still along 3.2 and 1.2 of Chisel/FIRRTL. Not sure who has a good sense of when these will be bumped, perhaps @azidar or @seldridge? |
|
@jackkoenig was working on bumping things here: #2399. |
Related issue: #2234
Type of change: feature request
Impact: API modification
Development Phase: implementation
Release Notes
This is an extension of @seldridge's work to move Rocket towards stages/phases.
I have so far tested making
vsimandemulator, but I'm not sure if I've missed any other calls to the generator...TODO
extendingChiselStagevs callingChiselStage? extendRunPhaseAnnotation?