Autoclonetype2

[jackkoenig]

Fixes #1803 (by coincidence as far as I can tell)

This change results in a speedup of ~40% and memory reduction 15-20% on large, typical designs. This also reduces memory use by 2-5x in some pathological cases.

Note that the diff is larger than it really is, I split ChiselPlugin.scala into 2 files. I did not make any changes to the ChiselComponent (which is really the NamingComponent). The bulk of my changes are in BundleComponent. I originally named it AutoCloneTypeComponent but I realized that it's not too big of an extension on this work to make the plugin also implement val elements in Bundles as well as solve the "Field problem". I almost did the latter, but it has some potential consequences that make me think it should be done on a major version boundary and I'd like to include this in v3.4.3. If we also implement val elements in the plugin, then there is no more runtime reflection necessary in Chisel.

Note the 2 new soft links in no-plugin-tests, reflective autoclonetype will continue being tested with all of the old tests. Those tests also apply to the new implementation along with some new tests I added.

I did add an assertion to reflective autoclonetype with the purpose of checking whether the plugin-generated version is actually in use. The cost of this assertion should be basically free compared to reflection; furthermore, users should use the plugin anyway for a massive performance improvement (to be measured).

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

• bug fix
• performance improvement (no more collecting stack traces for every created bundle!)
• code refactoring (the plugin is a bit opaque, it is much clearer than the reflective version)

API Impact

This expands the autoclonetype API to support Bundles with parameters that are not vals. It also can generate cloneTypes for anonymous classes with non-Bundle/Module outer classes.

Backend Code Generation Impact

No impact

Desired Merge Strategy

• Rebase (I'd like to keep the 4 commits)

Release Notes

Generate cloneType methods for Bundles in the compiler plugin when provided -P:chiselplugin:useBundlePlugin as a scalacOption.This feature is disabled by default because it is a breaking change to implement cloneType for any non-final Bundle (a child class extending the given Bundle could rely on reflective autoclonetype but will now call the newly implemented one in the superclass instead). Users who intend to publish libraries should not enable the feature until updating to Chisel 3.5. Everyone else should use it beginning in Chisel 3.4.3.

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels?
• Did you mark the proper milestone (3.2.x, 3.3.x, 3.4.0, 3.5.0) ?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you mark as Please Merge?

[jackkoenig]

For the record, I think the CI failure is unrelated. The test appears to be failing on master, it might be due to a change in FIRRTL. I'll debug soonish but may not be able to get to it till early next week.

[azidar]

LGTM, minor changes (add newlines to new files)

[azidar]

Looks stellar! Great job, this is very exciting and will have enormous impact on runtimes of large designs. Very exciting!!!

[jackkoenig]

New TODO. I'd still like to backport this to 3.4.x, but I have realized (by lucky accident) that this PR currently is not binary compatible. It's not that it changes any APIs and it doesn't (or at least, shouldn't) fail the MiMa check, but it turns out that implementing cloneType on a non-final Bundle is not binary compatible. This is due to the fact that any Bundles that extend your Bundle may have been relying on reflective autoclonetype are now calling your method which will result in a runtime error. Because this generates a bunch of cloneTypes (well _cloneTypeImpl but the exact same idea applies), this would be a problem for anyone extending any Bundles in chisel3.util, for example, rocket-chip.

The solution that allows users to still benefit from this without having a binary incompatible change in 3.4.3 is to make this behavior optional via an argument to the plugin, off by default. This Chisel itself will not use the feature in this PR which can be backported, and then will use it on master looking toward 3.5.

[mwachs5]

This Chisel itself will not use the feature in this PR which can be backported, and then will use it on master looking toward 3.5.

The implication being that users of this chisel can turn the plugin on for their code, and still link against the 3.4.x Chisel compiled with this bit of the plugin turned off, right?

[jackkoenig]

The implication being that users of this chisel can turn the plugin on for their code, and still link against the 3.4.x Chisel compiled with this bit of the plugin turned off, right?

Exactly, Chisel itself will remain binary compatible, anyone who wants to write a binary compatible library should keep this feature disabled, but anyone who wants the performance benefit in their own code can turn it on.

[ducky64]

I can't say I understand what all of this is doing, but it look fine.

Otherwise, a high-level description of the transform / plugin behavior, ideally in scaladoc, would be helpful as a guide for reading through the implementation.

[jackkoenig]

Feedback resolved and new option added so that this feature is opt-in rather than opt-out (for Chisel 3.4.x). It is not used by chisel3 itself yet, and the binary and source compatibility issue related to this is now tested.

I intend to merge this shortly and let it backport, and then create a follow on PR for Chisel 3.5 that uses the plugin, perhaps changing the option to opt-out rather than opt-in.