Modify and optimize performance of propagate annotations

[jackkoenig]

This is basically just deletes all of the logic we had determining which annotations were added or deleted between transforms. This does not change any Scala APIs, but it very much changes what sorts of DeletedAnnotations are created. The deleted behavior could be useful for debugging and technically could be relied upon someone, but my hunch is that no one cares and we're harming performance and burning carbon for nothing. If it turns out someone relies on this, I think we should add it back under some sort of "debug" annotation.

This change only really affects annotation-heavy flows. I found that with small designs with moderate numbers of annotations, it improves performance ~2-7%. For large designs with tons of annotations however, it improved performance by 34% (15m45 -> 10m20s)

This is a bit aggressive, but I think it's worth it.

Contributor Checklist

• [NA] Did you add Scaladoc to every public function/method?
• [NA] Did you update the FIRRTL spec to include every new feature/behavior?
• 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 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
• code cleanup

API Impact

Annotations that are deleted between transforms are no longer automatically captured as DeletedAnnotations

Backend Code Generation Impact

No direct impact

Desired Merge Strategy

• Squash

Release Notes

When a transform deletes an Annotation, a DeletedAnnotation is no longer created. Furthermore, debug-mode logging no longer captures any information about annotation changes between transforms. If any of this behavior is desired, please file an issue to restore it under a debug flag.

Reviewer Checklist (only modified by reviewer)

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

[mwachs5]

So this means that DeletedAnnotations will no longer show up in the annotation files when running with --trace?

[jackkoenig]

So this means that DeletedAnnotations will no longer show up in the annotation files when running with --trace?

That is correct. I personally find them annoying when debugging but I grant others may disagree. Adding them back under --debug or --trace is a reasonable suggestion, but I don't want to do the work unless there's a compelling use case.

[seldridge]

DeletedAnnotation is probably an anti-pattern and we should just remove it. There was old logic in Chisel's Driver that relied on this (examining deleted annotations to extract information). However, with Driver gone, there is no reason to keep this around.

What would make sense is to have special handling for -ll trace or we actually have a --debug option which is intentionally running slower, but providing more information. E.g., if you turn on --debug, it will wrap all transforms with an outer transform that records annotations before the transform and tells you what was deleted after the transform runs.

[jackkoenig]

I've revived this PR, it should be passing tests now. I know this is a bit of a change, but I think it's worth including in 1.5.0-RC2 and then if people have issues with it we can either revert or add an option to include the old DeletedAnnotation behavior.

[ekiwi]

LGTM

I was never a fan of the "DeletedAnnotation". Seemed like a memory leak to me.

[jackkoenig]

Lets go for it. We can revert or fix later if it's an issue

[jackkoenig]

I believe this PR has exposed a previously hidden bug, you can see it manifest in the chisel3 integration tests: https://github.com/chipsalliance/chisel3/runs/4563876893?check_suite_focus=true

It also manifests in chiseltest's own tests... I'm looking into it.