Add safer Chisel annotation API, deprecate old ones #4643
Conversation
jackkoenig
added
the
Deprecation
| 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.
The new one enables safety checks and smarter logic for views.
jackkoenig
force-pushed
the
safer-annotations
branch
from
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?
| "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.
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.