Add ChiselPhase, Stop writing files in ChiselStage$ methods, Expand ChiselStage$ helpers #1566
Conversation
This is a backwards compatible change from 3.3.2, why are we removing it? |
| val targets: Seq[Dependency[Phase]] = | ||
| Seq( Dependency[chisel3.stage.phases.Checks], | ||
| Dependency[chisel3.stage.phases.AddImplicitOutputFile], | ||
| Dependency[chisel3.stage.phases.AddImplicitOutputAnnotationFile], | ||
| Dependency[chisel3.stage.phases.MaybeAspectPhase], | ||
| Dependency[chisel3.stage.phases.Convert], | ||
| Dependency[chisel3.stage.phases.MaybeFirrtlStage] ) | ||
|
|
||
| final lazy val phaseManager = new PhaseManager(targets) { | ||
| override val wrappers = Seq( (a: Phase) => DeletedWrapper(a) ) | ||
| } | ||
| final lazy val phaseManager = new ChiselPhase |
There was a problem hiding this comment.
I don't think we should remove this API without first deprecating. It is used by rocket-chip after all: https://github.com/chipsalliance/rocket-chip/blob/d71c5a674275676b9c870a20cb0a42cd039a4115/src/main/scala/system/RocketChipStageGenerator.scala#L15
I dunno how many other users there might be, but I'd say preserve it by keeping phaseManager the same and then implement targets as def targets: Seq[Dependency[Phase]] = (new ChiselPhase).targets, then it can still be overridden to affect phaseManager, feel free to deprecate it though, instead suggesting composition I assume?
There was a problem hiding this comment.
Good catch. Updated in the rebase. There's annoyingly no good Scala 2.11 compatible way to deprecate this. @deprecatedOverriding is what we want, but it's a 2.12 API addition. (And just using @deprecated produces no warnings if you override it.)
That said, I feel like there are some warts with the Stage API that can be better handled with a flat structure. So, I wouldn't sweat this and would rather put more thought into how to better express this flat structure (which would likely be a larger update to the Stage API.)
Signed-off-by: Schuyler Eldridge <[email protected]>
Switch from a one-off PhaseManager inside ChiselStage to actually using the newly added ChiselPhase. This removes the targets method (and API) from ChiselStage. Signed-off-by: Schuyler Eldridge <[email protected]>
Change the ChiselStage companion object methods, elaborate and convert, to not write files. Under the hood, these are switched from using ChiselStage (which, like all phases, will write files) to using ChiselPhase. Signed-off-by: Schuyler Eldridge <[email protected]>
Modify existing ChiselStage object method tests to check that no files are written. Signed-off-by: Schuyler Eldridge <[email protected]>
This adds additional methods to the ChiselStage object for going directly from a Chisel module to a string including: CHIRRTL, high FIRRTL IR, Verilog, and SystemVerilog. Differing from their ChiselStage class counterparts, these take no arguments other than the module and write no files. Signed-off-by: Schuyler Eldridge <[email protected]>
Signed-off-by: Schuyler Eldridge <[email protected]>
seldridge
force-pushed
the
driver-methods
branch
from
3311720 to
66aa0c9
Compare
seldridge
changed the title
seldridge
force-pushed
the
driver-methods
branch
from
f658bcd to
5f44327
Compare
Signed-off-by: Schuyler Eldridge <[email protected]>
seldridge
force-pushed
the
driver-methods
branch
from
5f44327 to
00e11b2
Compare
|
@jackkoenig: One minor update to this. I changed tests that were using the class methods to using the object methods to prevent unnecessary file writing. Still okay to merge? |
|
Sorry for the delay, this looks great! |
Adds a new
private[chisel3]phase calledChiselPhasewhich does the work of the one-off phase manager inside the existingChiselStage. This is then used insideChiselStage.ChiselStagecompanion object methods,elaborateandconvert, are then switched to useChsielStageso that they stop writing files.This adds new APIs to the
ChiselStageobject that write no files:emitChirrtl(gen: => RawModule): StringemitFirrtl(gen: => RawModule): StringemitVerilog(gen: => RawModule): StringemitSystemVerilog(gen: => RawModule): StringTests are added, thanks to Java security manager magic, to assert that these methods do, in fact, write no files. Existing tests that use the
ChiselStageclass are modified to use the object if they are only checking a string output (and have no need to generate files).Commentary
The new methods which run the FIRRTL compiler have to not use
FirrtlStage(as that will write files), but this patch series shows how to use aPhaseManagerto piece together a custom solution out of existingPhases. I think that this hints at an overall better API than the omnibusStagewhich is opinionated in the fixed set ofPhasesthat it wants to run before and after your code.tl;dr: I think a flat structure is better than nesting of stages and it's worthwhile to think about how to refactor things to work this way.
Contributor Checklist
docs/src?Type of Improvement
API Impact
None.
Backend Code Generation Impact
None.
Desired Merge Strategy
Release Notes
Add additional helper methods to ChiselStage object: emitChirrtl, emitFirrtl, emitVerilog, emitSystemVerilog. These new methods, as well as existing ChiselStage object methods, elaborate and convert, will no longer write files.
Reviewer Checklist (only modified by reviewer)
Please Merge?