Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIRRTL][Dedup] Alter dedup group handling, avoid exponential behavior. #6985

Merged
merged 2 commits into from
May 3, 2024

Conversation

dtzSiFive
Copy link
Contributor

Transmute these annotations to simple (temporary) attributes on modules.
This causes the desired behavior for hashing and since this must match, when deduplicating no work is needed.

Presently, as annotations, each module dedup'd into the group walks all annotations and "adds context" (makes them non-local) and interns a new annotation array with those annotations. At the end of the pass, all dedupGroup annotations are removed.

This causes a lot of unnecessary symbols, hierpaths, annotations, and arrays of annotations as well as some quadratic behavior.

DedupGroup annotations are commonly on every single module, so for those designs large dedup groups scaled poorly.

Instead, drop these annotations immediately and add as a simple named attribute indicating the group.

Drop the (temporary) dedup group attributes at end of pass, as they are just an in-IR way to track this per-module during the pass.

Fixes #6979.


Benchmarking (n=10, median) on some quick synthetic examples put together, FWIW (top has instances of the many identical empty modules):

Name Modules Instances real before (s) real after (s) Before/After maxrss before (kb) maxrss after (kb) Before/After
Groups 3001 3000 1.035 0.311 3.33 124,096 72,944 1.70
Groups_3x 3001 9000 2.237 0.361 6.20 216,696 77,456 2.80
Many 10001 30000 33.349 3.461 9.64 1,802,784 475,366 3.79

Significantly improves performance where there are many instances of deduplicated modules and there are dedup group annotations.

…ior.

Transmute these annotations to simple (temporary) attributes on modules.

This causes the desired behavior for hashing and since this must
match, when deduplicating no work is needed.

Presently, as annotations, each module dedup'd into the group
walks all annotations and "adds context" (makes them non-local)
and interns a new annotation array with those annotations.
At the end of the pass, all dedupGroup annotations are removed.

This causes a lot of unnecessary symbols, hierpaths, annotations, and arrays of
annotations as well as some quadratic behavior.

DedupGroup annotations are commonly on every single module,
so for those designs large dedup groups scaled poorly.

Instead, drop these annotations immediately and add as a simple
named attribute indicating the group.

Drop the (temporary) dedup group attributes at end of pass,
as they are just an in-IR way to track this per-module
during the pass.

Fixes llvm#6979.
Pre-existing that caused these to persist to LowerToHW which
generated warnings about them (on non-fmoduleop fmodulelike's).

Now, prune the attribute added (on all) so they don't leak past.
@youngar
Copy link
Member

youngar commented May 3, 2024

Just to connect the dots, this is similar to what we do to prevent memories in the testharness and (i think) prefix-module-groups from deduping with each other: #2968

edit: looks like the groupId stuff was replaced with string prefixs: #4952

Copy link
Member

@youngar youngar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we talked about offline, one idea for the future is to change the annotation handler to attach this attribute (in LowerAnnotations) instead of doing it here. Change looks great to me, thanks!

@dtzSiFive dtzSiFive merged commit 62fbb3a into llvm:main May 3, 2024
4 checks passed
@dtzSiFive dtzSiFive deleted the feature/dedup-group-anno-perf branch May 3, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FIRRTL] Performance Degradation in Dedup Runtime between Chisel3.6 and Chisel6
2 participants