New enhanced API for specifying Chisel to Firrtl Annotations #2628

girishpai · 2022-07-12T17:41:24Z

Contributor Checklist

Did you add Scaladoc to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous printlns/debugging code?
Did you specify the type of improvement?
Did you add appropriate documentation in docs/src?
Did you state the API impact?
Did you specify the code generation impact?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

Type of Improvement

new feature / API

High level Changes

With the intent to ensure no existing code breaks / needs an update - a different variable (called newAnnotations) was used. Also wherever this is used a default value of Seq.empty was provided in the function argument list - again to avoid updating existing code.
A new trait called ChiselMultiAnnotaiton was created with toFirrtl method - which returns a Seq of Annotations.

@jackkoenig this is based on our agreed upon strategy (creating a totally new trait as opposed to adding a new method in the existing one)

API Impact

No impact to current code. The current API is useful to generate annotations containing information which will only be available after a module elaborates. However this is limited since we can only generate a single annotation per call. In some cases we need ability to generate a Seq of annotations.

Backend Code Generation Impact

No change to generated verilog.

Desired Merge Strategy

Squash

Release Notes

New API to convert a single Chisel Annotation into a Seq of FirrtlAnnotations

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels?
Did you mark the proper milestone (Bug fix: 3.4.x, [small] API extension: 3.5.x, API modification or big change: 3.6.0)?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you do one of the following when ready to merge:
- Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
- Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

core/src/main/scala/chisel3/internal/firrtl/IR.scala

mwachs5 · 2022-07-12T19:06:56Z

Can you fill out the template, especially in the Release notes say that you added a new API for Converting a single Chisel Annotation to a Sequence of FIRRTL annotations?

mwachs5

Shouldn't there be something when we do Convert to FIRRTL that actually looks at these newAnnotations?

https://github.com/chipsalliance/chisel3/blob/2b48fd15a7711dcd44334fbbc538667a102a581a/src/main/scala/chisel3/stage/phases/Convert.scala#L30

core/src/main/scala/chisel3/internal/firrtl/IR.scala

seldridge

Generally looks good. Some ideas/suggestions throughout.

core/src/main/scala/chisel3/Annotation.scala

src/test/scala/chiselTests/NewAnnotationsSpec.scala

jackkoenig · 2022-07-12T20:29:44Z

core/src/main/scala/chisel3/internal/firrtl/IR.scala

+  annotations:    Seq[ChiselAnnotation],
+  renames:        RenameMap,
+  newAnnotations: Seq[ChiselToFirrtlAnnotations] = Seq.empty) {


Adding fields to case classes is both a binary incompatible and source incompatible change. This would not be an issue if we had properly made all of these classes private.... but we didn't and some users do notice.

To safely backport this to 3.5.x (which I presume is your plan) we either need to use dataclass to give case class-like behavior in a more friendly way for making modifications like this, or we need to add these new arguments to a 2nd parameter list and implement equals and hashcode ourselves.

@jackkoenig let me explore that - I am assuming since the annotations val is publicly visible - the change suggested by @seldridge to make annotations Seq[ChiselToFirrtlAnnotations] instead of Seq[ChiselAnnotation] also is disruptive?

@jackkoenig please take a look - have used dataclass - however not sure if I have handled copy properly (also do we need an unapply method? )

Copy should exclude any new methods (any after @since), also please add "Chisel 3.5.4" to the since just to make it obvious. You also do need to define unapply. To check if your changes are binary compatible, you can try making them against 3.5.x and then running sbt +mimaReportBinaryIncompatibilities

See https://github.com/chipsalliance/firrtl/blob/c7403e80f965aa3c6f65b176dbb56dbb905cbb31/src/main/scala/firrtl/ir/IR.scala#L769 for example of the copy not having any new fields and of implementing unapply yourself (again not including the new fields).

@jackkoenig did what you suggested - and I am seeing similar errors as mentioned in the PR you linked after my update - can these be ignored?

error] chisel3-core: Failed binary compatibility check against edu.berkeley.cs:chisel3-core_2.12:3.5.3! Found 4 potential problems (filtered 8) [error] * class chisel3.internal.firrtl.Circuit is declared final in current version [error] filter with: ProblemFilters.exclude[FinalClassProblem]("chisel3.internal.firrtl.Circuit") [error] * static method tupled()scala.Function1 in class chisel3.internal.firrtl.Circuit does not have a correspondent in current version [error] filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.internal.firrtl.Circuit.tupled") [error] * static method curried()scala.Function1 in class chisel3.internal.firrtl.Circuit does not have a correspondent in current version [error] filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("chisel3.internal.firrtl.Circuit.curried") [error] * the type hierarchy of object chisel3.internal.firrtl.Circuit is different in current version. Missing types {scala.runtime.AbstractFunction4} [error] filter with: ProblemFilters.exclude[MissingTypesProblem]("chisel3.internal.firrtl.Circuit$")

@jackkoenig have incorporated your changes to resolve the compatibility issues - the check is still running locally - and so far so good - will update once it ends (hopefully passes).

It passed!

sbt +mimaReportBinaryIssues .... [success] Total time: 22 s, completed Jul 13, 2022 11:25:27 AM [info] Setting Scala version to 2.13.7 on 6 projects. [info] Reapplying settings... [info] set current project to chisel3 (in build file:/scratch/girishp/vscode/clone_23jun/federation/rocket-chip/chisel3/) [success] Total time: 0 s, completed Jul 13, 2022 11:25:28 AM [info] Setting Scala version to 2.13.8 on 6 projects. [info] Reapplying settings... [info] set current project to chisel3 (in build file:/scratch/girishp/vscode/clone_23jun/federation/rocket-chip/chisel3/) [success] Total time: 0 s, completed Jul 13, 2022 11:25:29 AM [info] Reapplying settings... [info] set current project to chisel3 (in build file:/scratch/girishp/vscode/clone_23jun/federation/rocket-chip/chisel3/)

This is on 3.5.x ....

core/src/main/scala/chisel3/internal/firrtl/IR.scala

core/src/main/scala/chisel3/Annotation.scala

core/src/main/scala/chisel3/internal/firrtl/IR.scala

(cherry picked from commit 4b10cf7)

…2631) (cherry picked from commit 4b10cf7) Co-authored-by: Girish Pai <girish.pai@sifive.com>

girishpai added 2 commits July 12, 2022 10:32

Initial version of new annotation api

9508705

Refactor;Add test

5d59246

girishpai marked this pull request as ready for review July 12, 2022 17:44

Add comment

9c5389c

mwachs5 reviewed Jul 12, 2022

View reviewed changes

core/src/main/scala/chisel3/internal/firrtl/IR.scala Show resolved Hide resolved

mwachs5 added this to the 3.5.x milestone Jul 12, 2022

mwachs5 reviewed Jul 12, 2022

View reviewed changes

girishpai commented Jul 12, 2022

View reviewed changes

core/src/main/scala/chisel3/internal/firrtl/IR.scala Show resolved Hide resolved

seldridge reviewed Jul 12, 2022

View reviewed changes

core/src/main/scala/chisel3/Annotation.scala Show resolved Hide resolved

core/src/main/scala/chisel3/Annotation.scala Outdated Show resolved Hide resolved

src/test/scala/chiselTests/NewAnnotationsSpec.scala Show resolved Hide resolved

jackkoenig reviewed Jul 12, 2022

View reviewed changes

Use dataClass

0088680

girishpai commented Jul 12, 2022

View reviewed changes

core/src/main/scala/chisel3/internal/firrtl/IR.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Jul 12, 2022

View reviewed changes

core/src/main/scala/chisel3/Annotation.scala Outdated Show resolved Hide resolved

girishpai commented Jul 12, 2022

View reviewed changes

core/src/main/scala/chisel3/internal/firrtl/IR.scala Show resolved Hide resolved

girishpai added 2 commits July 12, 2022 20:32

Proper version of copy/unapply

d91ccb3

Changing name of new annotation

f322be9

girishpai requested a review from jackkoenig July 13, 2022 15:46

girishpai added 2 commits July 13, 2022 09:34

Update test

4005116

Incorporate Jack's changes to resolve binary compatability issues

cf56ba6

jackkoenig approved these changes Jul 13, 2022

View reviewed changes

jackkoenig merged commit 4b10cf7 into chipsalliance:master Jul 13, 2022

mergify bot pushed a commit that referenced this pull request Jul 13, 2022

New enhanced API for specifying Chisel to Firrtl Annotations (#2628)

cb0e1e7

(cherry picked from commit 4b10cf7)

mergify bot mentioned this pull request Jul 13, 2022

New enhanced API for specifying Chisel to Firrtl Annotations (backport #2628) #2631

Merged

mergify bot added the Backported This PR has been backported label Jul 13, 2022

mergify bot added a commit that referenced this pull request Jul 13, 2022

New enhanced API for specifying Chisel to Firrtl Annotations (#2628) (#…

dbffb87

…2631) (cherry picked from commit 4b10cf7) Co-authored-by: Girish Pai <girish.pai@sifive.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New enhanced API for specifying Chisel to Firrtl Annotations #2628

New enhanced API for specifying Chisel to Firrtl Annotations #2628

girishpai commented Jul 12, 2022 •

edited

Loading

mwachs5 commented Jul 12, 2022

mwachs5 left a comment

seldridge left a comment

jackkoenig Jul 12, 2022

girishpai Jul 12, 2022 •

edited

Loading

girishpai Jul 12, 2022

jackkoenig Jul 12, 2022

girishpai Jul 13, 2022

girishpai Jul 13, 2022

girishpai Jul 13, 2022

girishpai Jul 13, 2022

New enhanced API for specifying Chisel to Firrtl Annotations #2628

New enhanced API for specifying Chisel to Firrtl Annotations #2628

Conversation

girishpai commented Jul 12, 2022 • edited Loading

Contributor Checklist

Type of Improvement

High level Changes

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

mwachs5 commented Jul 12, 2022

mwachs5 left a comment

Choose a reason for hiding this comment

seldridge left a comment

Choose a reason for hiding this comment

jackkoenig Jul 12, 2022

Choose a reason for hiding this comment

girishpai Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

girishpai Jul 12, 2022

Choose a reason for hiding this comment

jackkoenig Jul 12, 2022

Choose a reason for hiding this comment

girishpai Jul 13, 2022

Choose a reason for hiding this comment

girishpai Jul 13, 2022

Choose a reason for hiding this comment

girishpai Jul 13, 2022

Choose a reason for hiding this comment

girishpai Jul 13, 2022

Choose a reason for hiding this comment

girishpai commented Jul 12, 2022 •

edited

Loading

girishpai Jul 12, 2022 •

edited

Loading