-
Notifications
You must be signed in to change notification settings - Fork 615
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Aspect-Oriented Programming for Chisel #1077
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks almost ready. Code looks good, just a few style
notations. Tests all seem to pass to me.
chiselFrontend/src/main/scala/chisel3/aop/AspectTransform.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
No huge concerns. Some questions and minor suggestions. The injecting aspect is very slick.
More major questions:
- This includes some printf modifications? Was this intended and can that be factored into a separate PR?
- There are somewhat liberal uses of
TypeTag
, but I can't convince myself that they're necessary (possibly due to my ignorance). Can you elaborate on what's going on there?
I did not examine Select
in much detail and completely glossed over:
Select.searchWhens
Select.connectionsTo
Co-Authored-By: Schuyler Eldridge <schuyler.eldridge@ibm.com>
Co-Authored-By: Schuyler Eldridge <schuyler.eldridge@ibm.com>
I believe I've addressed all your comments! Some of the TypeTags were unnecessary - the only thing is they may be necessary for subclasses of Aspect. I think, however, that's something we can talk about in a tutorial or people can pick-up from the examples that exist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
@@ -46,8 +46,9 @@ case class ChiselGeneratorAnnotation(gen: () => RawModule) extends NoTargetAnnot | |||
|
|||
/** Run elaboration on the Chisel module generator function stored by this [[firrtl.annotations.Annotation]] | |||
*/ | |||
def elaborate: ChiselCircuitAnnotation = try { | |||
ChiselCircuitAnnotation(Builder.build(Module(gen()))) | |||
def elaborate: AnnotationSeq = try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks testers2 which expects to be able to reference a circuit:
[info] Compiling 29 Scala sources to .../testers2/target/scala-2.11/classes ...
[error] /vm/home/jenkins/workspace/chisel3-pl_2/testers2/src/main/scala/chisel3/tester/backends/treadle/TreadleExecutive.scala:30:49: value circuit is not a member of firrtl.AnnotationSeq
[error] val circuit = generatorAnnotation.elaborate.circuit
[error] ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're still pre-3.2, is it safe to fix this on the testers2 side? Something like:
val circuit = generatorAnnotation.elaborate._1.circuit
Type of change: New Feature Implementation
Impact: API addition (no impact on existing code)
Development Phase: Ready to merge, please review!
Motivation
The idea behind this work is to increase the direct reuse of Chisel hardware libraries. A common and unfortunate pattern we see in Chisel land is the forking of Chisel library repositories, e.g. the rocket-chip forks required by every Berkeley tape-out, as well as FireSim's rocket-chip use (while not a fork, requires a custom build system).
The fundamental problem is that these projects require adding additional instrumentation, annotations, or minor modifications to a Chisel3 library that are both design- and backend-specific. One example is annotating signals for Midas transformations; this information does not belong in rocketchip (because it is backend-dependent), nor does it belong in Midas (because it is design-specific). What then ends up happening is that FireSim (the project that applies Midas to rocket-chip) must fork rocket-chip to add Midas annotations.
This example is a form of a problem known in software-land as dependency injection, and has many potential solutions, one of which is aspect-oriented programming. While its now generally regarded as an anti-pattern in software, I'm hypothesizing that not only does this paradigm make sense in the hardware-generation context, its already present in the form of tcl scripts for CAD tools and is being exacerbated by the power of Chisel parameterization.
This PR contains modifications to Chisel that enable aspect-oriented dependency injection. It will be experimental (and thus no claims for preserving backwards-compatibility), but we want to see whether this feature can indeed address the previously described reuse problem.
Next Steps
For some example use-cases demonstrating the power of Chisel Aspects, check out the following tests in the aop branch of riscv-mini:
For some concrete implementations of Chisel Aspects, check out the following. Please note that all of these aspects do not require any modifications to riscv-mini's source code!!!
Outline of Software Architecture
Aspect[T]
---T
is type of the top-level moduletoAnnotation
function prior to executing FIRRTL. These generated FIRRTL annotations determine the behavior of the aspect. Aspect ordering is relative only to how the annotations are ordered/consumed by FIRRTL transforms (e.g. every aspect is lazily applied to the circuit whenever the FIRRTL complier runs a transform that consumes an aspect-generated annotation). This is similar to how annotations work in Chisel - the order a signal is annotated does not matter; what matters is when the annotation is consumed.def toAnnotation(dut: DUT): Annotation
TODO: