Chisel plugin bundle elements handler #2306

chick · 2021-12-16T21:52:09Z

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

Performance improvement
Code refactoring

API Impact

Could affect unusual Bundle constructions.
Some error messages have been changed

Backend Code Generation Impact

This should not impact code generation

Desired Merge Strategy

Release Notes

This eliminates the use of reflection in Aggregate.scala when building the elements method of a Bundle.
This should result in improvement in elaboration times for large designs.

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels?
Did you mark the proper milestone (Bug fix: 3.3.x, [small] API extension: 3.4.x, API modification or big change: 3.5.0)?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you mark as Please Merge?

core/src/main/scala/chisel3/BundleComparator.scala

src/main/scala/chisel3/util/HasDemoBundle.scala

core/src/main/scala/chisel3/Aggregate.scala

src/test/scala/chiselTests/BundleSpec.scala

src/main/scala/chisel3/util/HasDemoBundle.scala

src/test/scala/chiselTests/BundleElementsSpec.scala

plugin/src/main/scala/chisel3/internal/plugin/ChiselPlugin.scala

plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala

docs/src/appendix/chisel-plugin.md

plugin/src/main/scala/chisel3/internal/plugin/ChiselPlugin.scala

src/test/scala/chiselTests/BundleElementsSpec.scala

core/src/main/scala/chisel3/Aggregate.scala

core/src/main/scala/chisel3/BundleComparator.scala

docs/src/appendix/chisel-plugin.md

plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala

jackkoenig · 2022-01-20T20:45:40Z

docs/src/appendix/chisel-plugin.md

+The Chisel plugin provides some operations that are too difficult, or not possbile, 
+to implement through regular Scala code.
+
+## Compiler plugin operations
+1. Automatically generates the `cloneType` methods of Bundles
+2. Records that this plugin ran on a bundle by adding a method `_usingPlugin: Boolean = true`
+3. Generates a function that returns list of the hardware fields in a bundle `_elementsImpl`
+


I think this is useful information, but it feels like a weird mix of "useful to users" and "useful only to contributors". I think it would be better to either split this into 2 files, or at least call out with a ## level title ("Notes for Contributors") or something.

Agreed -- for example should this appear on the website?

I'd generally say the contributor only stuff should not live on the website, but I'm not to fussed either way, it just needs to be clear what is what.

@jackkoenig @mwachs5 I agree this is weird. I moved this purposely into the README instead of placing it where it would be picked up by website. This should be a short term option that quickly (maybe by 3.5.2) becomes the default.
I tried to simplify and condense things a bit. Not sure how much more to do here

core/src/main/scala/chisel3/BundleComparator.scala

docs/src/appendix/appendix.md

docs/src/appendix/chisel-plugin.md

build.sbt

src/test/scala/chiselTests/BundleElementsSpec.scala

… `BundleComponent` For each `Bundle` find the relevant visible Chisel field members and construct a hard-coded list of the elements and their types and implement as `_elementsImpl` For more details: See the docs/src/appendix/chisel-plugin.md Enable the Bundle element generator in the Plugin - Should be no change in API - Handles inheritance and mixins - Handles Seq[Data - Tests in BundleElementSpec

- completely refactored BundleElementsSpec to not require BundleComparator - Removed BundleComparator.scala - Based on PR review - renamed flags buildElements* to genBundleElements* - removed cargo cult comments - -

- changed build.sbt to set genBundleElements to be true - changed build.sc to set genBundleElements to be true

- compare data fields as well as names

- Moved `chisel-plugin.md` to `plugins/README.md` as it is strictly for chisel internals work - Implemented wording changes based on PR review comments

- Some small changes to BundleElementsSpec - Added a section of negative tests "plugin tests should fail properly in the following cases" - simplified assertElementsMatchExpected - Added an example of pathological bundle elements `val field: Any = UInt(22.W)`

jackkoenig · 2022-01-31T23:56:53Z

build.sc

@@ -58,7 +58,8 @@ trait CommonModule extends CrossSbtModule with PublishModule with ScalafmtModule
  override def scalacOptions = T {
    super.scalacOptions() ++ Agg(
      "-deprecation",
-      "-feature"
+      "-feature",
+      "-P:chiselplugin:genBundleElements"


This also needs to only affect the tests, put it in object test below.

jackkoenig · 2022-02-01T00:58:03Z

I have not done super robust benchmarking here, but for a pretty typical large SiFive design, this makes Chisel ~16% faster. Pretty good!

jackkoenig

LGTM! Excellent work @chick!

Adds generation of `Bundle.elements` method to the chores done by the compiler plugin For each `Bundle` find the relevant visible Chisel field members and construct a hard-coded list of the elements and their names implemented as `_elementsImpl` For more details: See plugins/README.md - Should be no change in API - Handles inheritance and mixins - Handles Seq[Data] - Tests in BundleElementSpec Co-authored-by: chick <chick.markley@sifive.com> Co-authored-by: Jack Koenig <koenig@sifive.com> (cherry picked from commit 237200a)

Adds generation of `Bundle.elements` method to the chores done by the compiler plugin For each `Bundle` find the relevant visible Chisel field members and construct a hard-coded list of the elements and their names implemented as `_elementsImpl` For more details: See plugins/README.md - Should be no change in API - Handles inheritance and mixins - Handles Seq[Data] - Tests in BundleElementSpec Co-authored-by: chick <chick.markley@sifive.com> Co-authored-by: Jack Koenig <koenig@sifive.com> (cherry picked from commit 237200a) Co-authored-by: Chick Markley <chick@qrhino.com>

chick added code improvement release issue Challenge Issue A nice-to-have but difficult to implement labels Dec 16, 2021

chick requested review from jackkoenig and mwachs5 December 16, 2021 21:52

chick self-assigned this Dec 16, 2021

mwachs5 reviewed Dec 16, 2021

View reviewed changes

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

jackkoenig reviewed Dec 16, 2021

View reviewed changes

src/main/scala/chisel3/util/HasDemoBundle.scala Outdated Show resolved Hide resolved

jackkoenig force-pushed the chisel-plugin-bundle-elements-handler branch 2 times, most recently from fdc952a to a2346bf Compare January 7, 2022 19:42

jackkoenig reviewed Jan 7, 2022

View reviewed changes

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

jackkoenig requested changes Jan 14, 2022

View reviewed changes

mwachs5 reviewed Jan 19, 2022

View reviewed changes

docs/src/appendix/chisel-plugin.md Outdated Show resolved Hide resolved

mwachs5 reviewed Jan 19, 2022

View reviewed changes

docs/src/appendix/chisel-plugin.md Outdated Show resolved Hide resolved

mwachs5 reviewed Jan 19, 2022

View reviewed changes

docs/src/appendix/chisel-plugin.md Outdated Show resolved Hide resolved

jackkoenig added this to the 3.5.x milestone Jan 20, 2022

chick force-pushed the chisel-plugin-bundle-elements-handler branch from f80f55a to 6d3c597 Compare January 20, 2022 19:55

jackkoenig reviewed Jan 20, 2022

View reviewed changes

mwachs5 reviewed Jan 20, 2022

View reviewed changes

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

mwachs5 reviewed Jan 20, 2022

View reviewed changes

docs/src/appendix/appendix.md Outdated Show resolved Hide resolved

mwachs5 reviewed Jan 20, 2022

View reviewed changes

docs/src/appendix/chisel-plugin.md Outdated Show resolved Hide resolved

mwachs5 reviewed Jan 20, 2022

View reviewed changes

docs/src/appendix/chisel-plugin.md Outdated Show resolved Hide resolved

mwachs5 reviewed Jan 20, 2022

View reviewed changes

docs/src/appendix/chisel-plugin.md Outdated Show resolved Hide resolved

mwachs5 reviewed Jan 20, 2022

View reviewed changes

docs/src/appendix/chisel-plugin.md Outdated Show resolved Hide resolved

mwachs5 reviewed Jan 20, 2022

View reviewed changes

docs/src/appendix/chisel-plugin.md Outdated Show resolved Hide resolved

jackkoenig reviewed Jan 29, 2022

View reviewed changes

build.sbt Outdated Show resolved Hide resolved

jackkoenig reviewed Jan 29, 2022

View reviewed changes

src/test/scala/chiselTests/BundleElementsSpec.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Jan 31, 2022

View reviewed changes

src/test/scala/chiselTests/BundleElementsSpec.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Jan 31, 2022

View reviewed changes

src/test/scala/chiselTests/BundleElementsSpec.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Jan 31, 2022

View reviewed changes

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

chick added 7 commits January 31, 2022 11:42

- fixed code formatting problem

efb348f

- changed javac to scalac in plugin docs

ba9cad3

- completely refactored BundleElementsSpec to not require BundleComparator - Removed BundleComparator.scala - Based on PR review - renamed flags buildElements* to genBundleElements* - removed cargo cult comments - -

- changed default for genBundleElements to be false

46d7622

- changed build.sbt to set genBundleElements to be true - changed build.sc to set genBundleElements to be true

- Made the tests stronger in BundleElementsSpec

b076a38

- compare data fields as well as names

- Removed docs reference to chisel-plugin.md

127ff4e

- Moved `chisel-plugin.md` to `plugins/README.md` as it is strictly for chisel internals work - Implemented wording changes based on PR review comments

jackkoenig force-pushed the chisel-plugin-bundle-elements-handler branch from b84dc3a to a118acc Compare January 31, 2022 23:56

jackkoenig reviewed Jan 31, 2022

View reviewed changes

chick and others added 2 commits February 1, 2022 10:18

- Make mill only enable bundle elements plugin in Test

90cdf56

Merge branch 'master' into chisel-plugin-bundle-elements-handler

7bddf85

jackkoenig approved these changes Feb 1, 2022

View reviewed changes

jackkoenig marked this pull request as ready for review February 1, 2022 18:24

jackkoenig enabled auto-merge (squash) February 1, 2022 18:28

jackkoenig added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Feb 1, 2022

jackkoenig merged commit 237200a into master Feb 1, 2022

jackkoenig deleted the chisel-plugin-bundle-elements-handler branch February 1, 2022 19:10

mergify bot mentioned this pull request Feb 1, 2022

Chisel plugin bundle elements handler (backport #2306) #2380

Merged

mergify bot added the Backported This PR has been backported label Feb 1, 2022

jackkoenig mentioned this pull request Feb 3, 2022

Fix bundle elements performance regression #2396

Merged

14 tasks

sequencer mentioned this pull request May 3, 2022

Deprecate BitSet.terms API in Chisel 3.5 #2517

Open

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chisel plugin bundle elements handler #2306

Chisel plugin bundle elements handler #2306

chick commented Dec 16, 2021

jackkoenig Jan 20, 2022

mwachs5 Jan 20, 2022

jackkoenig Jan 20, 2022

chick Jan 31, 2022

jackkoenig Jan 31, 2022

jackkoenig commented Feb 1, 2022

jackkoenig left a comment

Chisel plugin bundle elements handler #2306

Chisel plugin bundle elements handler #2306

Conversation

chick commented Dec 16, 2021

Contributor Checklist

Type of Improvement

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

jackkoenig Jan 20, 2022

Choose a reason for hiding this comment

mwachs5 Jan 20, 2022

Choose a reason for hiding this comment

jackkoenig Jan 20, 2022

Choose a reason for hiding this comment

chick Jan 31, 2022

Choose a reason for hiding this comment

jackkoenig Jan 31, 2022

Choose a reason for hiding this comment

jackkoenig commented Feb 1, 2022

jackkoenig left a comment

Choose a reason for hiding this comment