Add safer Chisel annotation API, deprecate old ones #4643
Conversation
| object Circuit | ||
| extends scala.runtime.AbstractFunction7[String, Seq[Component], Seq[ChiselAnnotation], RenameMap, Seq[ | ||
| DefTypeAlias | ||
| ], Seq[Layer], Seq[DefOption], Circuit] { | ||
| def unapply(c: Circuit): Option[(String, Seq[Component], Seq[ChiselAnnotation], RenameMap, Seq[DefTypeAlias])] = { | ||
| Some((c.name, c.components, c.annotations, c.renames, c.typeAliases)) | ||
| } | ||
|
|
||
| def apply( | ||
| name: String, | ||
| components: Seq[Component], | ||
| annotations: Seq[ChiselAnnotation], |
There was a problem hiding this comment.
All of this was left over from previous needs to maintain binary compatibility. These APIs are all package private now so we can just delete them.
There was a problem hiding this comment.
Backing out this change for now because Circuit leaks as discussed in #4683. Will follow up tearing this out later.
fb3b032 to
7b4c3d9
Compare
|
I'm thinking maybe backport this deprecation to 6.x, then I can follow up with the API removal on main and then we can merge #4644 to get the performance improvement. |
| /** Create annotations. | ||
| * | ||
| * Avoid this API if possible. | ||
| * | ||
| * Anything being annotated must be passed as arguments so that Chisel can do safety checks. | ||
| * The caller is still responsible for calling .toTarget on those arguments in mkAnnos. | ||
| */ | ||
| def apply(targets: InstanceId*)(mkAnnos: => Seq[Annotation]): Unit = { | ||
| targets.foreach { | ||
| case d: Data => requireIsAnnotatable(d, "Data marked with annotation") | ||
| case _ => () | ||
| } | ||
| // TODO record views that need to be renamed | ||
| Builder.annotations += (() => mkAnnos) | ||
| } |
There was a problem hiding this comment.
Is there a way to do this which directly passes the targets to the annotation construction function directly?
For this to work, an Annotation (or a subclass of) would need to have a constructor that can build it from an InstanceId (or a Data).
I'm primarily worried about there being no check of the connection between the targets and the mkAnnos function as a user could put in incorrect targets or ignore them entirely to get the old, legacy behavior.
Can you think of a way to untangle this?
There was a problem hiding this comment.
Discussed offline--we could untangle this, but it's a lot of extra infrastructure around stuff that users aren't really supposed to be using anyway since user-defined annotations are not supported in CIRCT. This is slightly ugly but good enough.
| "Avoid custom annotations. If you must use annotations, new annotate.apply method that takes Data", | ||
| "Chisel 7.0" |
There was a problem hiding this comment.
Wrong deprecation message, just deprecate the annotation.
f19a35e to
eccc0cd
Compare
There was a problem hiding this comment.
So, the plan with this PR is to ignore the enum annotations and let them use the deprecated methods. We can then see about removing the annotations in a follow-on. Is that right?
There was a problem hiding this comment.
That's correct, I plan to backport this PR to 6.x (although with some parts backed out) and then delete the enum annotations (and deprecated annotate methods).
e765b25 to
b9a36f3
Compare
|
Converted to draft because it looks like |
b9a36f3 to
f645487
Compare
The new one enables safety checks and smarter logic for views.
f645487 to
6586441
Compare
The new one enables safety checks and smarter logic for views. (cherry picked from commit a95cfe4) # Conflicts: # build.mill # build.sbt # core/src/main/scala/chisel3/Annotation.scala # core/src/main/scala/chisel3/ChiselEnum.scala # core/src/main/scala/chisel3/Module.scala # core/src/main/scala/chisel3/dontTouch.scala # core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala # src/main/scala/chisel3/util/AttributeAnnotation.scala # src/main/scala/chisel3/util/experimental/Inline.scala # src/main/scala/chisel3/util/experimental/LoadMemoryTransform.scala # src/main/scala/circt/OutputDirAnnotation.scala # src/test/scala-2/chiselTests/util/SRAMSpec.scala # src/test/scala/chiselTests/AnnotatingDiamondSpec.scala # src/test/scala/chiselTests/experimental/hierarchy/Annotations.scala # src/test/scala/circtTests/stage/ChiselStageSpec.scala
✅ Backports have been created
|
…4697) * Add safer Chisel annotation API, deprecate old ones (#4643) The new one enables safety checks and smarter logic for views. (cherry picked from commit a95cfe4) # Conflicts: # build.mill # build.sbt # core/src/main/scala/chisel3/Annotation.scala # core/src/main/scala/chisel3/ChiselEnum.scala # core/src/main/scala/chisel3/Module.scala # core/src/main/scala/chisel3/dontTouch.scala # core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala # src/main/scala/chisel3/util/AttributeAnnotation.scala # src/main/scala/chisel3/util/experimental/Inline.scala # src/main/scala/chisel3/util/experimental/LoadMemoryTransform.scala # src/main/scala/circt/OutputDirAnnotation.scala # src/test/scala-2/chiselTests/util/SRAMSpec.scala # src/test/scala/chiselTests/AnnotatingDiamondSpec.scala # src/test/scala/chiselTests/experimental/hierarchy/Annotations.scala # src/test/scala/circtTests/stage/ChiselStageSpec.scala * Resolve backport conflicts * Waive false MiMa issues * Fix ScalaDoc --------- Co-authored-by: Jack Koenig <koenig@sifive.com>
This is required for the new Chisel annotations API: chipsalliance/chisel#4643
This was introduced in chipsalliance/chisel#4643 and was released in Chisel 6.7.0.
The `ChiselAnnotation` API is going away in Chisel 7. This API was introduced in chipsalliance/chisel#4643 and was released in Chisel 6.7.0.
This is required for the new Chisel annotations API: chipsalliance/chisel#4643
The `ChiselAnnotation` API is going away in Chisel 7. This API was introduced in chipsalliance/chisel#4643 and was released in Chisel 6.7.0. Resolves chipsalliance#18.
This is required for the new Chisel annotations API: chipsalliance/chisel#4643
Note that I tested that the deprecated versions of
annotatework by updating them first, making sure all tests pass, then removing all uses of ChiselAnnotation and ChiselMultiAnnotation except in ChiselEnum. The ones in ChiselEnum have duplication checking that I could replicate with the annotations, but is annoying. I figure we'd rather just remove those (and roll out firrtl enums? 🙏) so I just left them. They also do help test the deprecated APIs.This PR has two purposes:
(2) will be in a follow on PR.
One weirdness in the PR that I would appreciate review of is that the new API asks the caller to "report" all
InstanceIdsthat will be annotated. We really only currently care aboutDatabut I figure in the future we may want checks for other things. The awkwardness though, is thatInstanceIdincludesData,BaseModule, andMemBase, but notHierarchynor the new SRAM targets. So if we wanted more checks, we might need to change the API anyway in the future since users can't currently reportHierarchyorSRAMtargets being annotated. I could take a step back and try to find a way to unify all of these, or just leave it, or switch the API toDatato make it more clear that that is all we're checking. WDYT?Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Creating annotations in Chisel now requires reporting what
InstanceIdsare going to be annotated so that Chisel can do some safety checks.Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.