chipsalliance / chipsalliance/chisel

InjectingAspect and RunFirrtlTransformAnnotation Don't Play Well Together

Open
#1,674 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.