chipsalliance / chipsalliance/chisel
InjectingAspect and RunFirrtlTransformAnnotation Don't Play Well Together
- Dominant language
- Scala
- Stars
- 4.8k
- Forks
- 658
- Avg merge
- 18h 59m
- Merged PRs (30d)
- 14
Description
Any `RunFirrtlTransformAnnotation` (or any annotation that needs to pass information to a phase or transform that runs before `InjectingTransform`) will get seemingly dropped if you add it in an `InjectingAspect`.
This is because the `InjectingAspect` is packing the annotations inside an `InjectStatement` annotation. While I grok that this makes sense for an `InjectingAspect` where the new annotations may refer to *new* targets in the circuit that don't exist unless the new statements are added to the module, this is unintuitive behavior for a user.
Full example in [this Scastie snippet](https://scastie.scala-lang.org/Oiro2BsLQ3SlALu2KnuRdA).
I recall going back and forth with @azidar about this behavior and whether or not "dynamic scheduling" of transforms should be allowed. At the time, I think the discussion was about this exact situation and then having the transform order recomputed *after* the statements are added. While I'm still against that, it should probably either be allowed to do what I'm doing (any `RunFirrtlTransformAnnotation`s are used to schedule transforms downstream) or explicitly disallowed (any `RunFirrtlTransformAnnotation`s added cause an aspect-time error). The current situation of having these silently dropped is difficult to debug.
I do think that it would be good to have another type of aspect different from `InjectingAspect` ("add statements and annotations *after* the injecting transform") and `InspectingAspect` ("look at the circuit and return no annotations"). It seems like there's a third aspect that looks at specific modules and then adds annotations.
Narrowly, I should just write a custom Aspect. :)
**Type of issue**: bug report | feature request
**Impact**: unknown
**Development Phase**: request
**Other information**
**If the current behavior is a bug, please provide the steps to reproduce the problem:**
See Scastie snippet.
**What is the current behavior?**
`RunFirrtlTransformAnnotation`s added in an `InjectingAspect` are effectively ignored.
**What is the expected behavior?**
Either:
1. These should be added to `FirrtlStage`
2. These should cause an aspect-time error.
**Please tell us about your environment:**
Chisel 3.4.0
**What is the use case for changing the behavior?**
Better debugging for users. Let them know that transforms aren't going to be run.
Contributor guide
Assessment
This issue has not been assessed yet.