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.

Sign up for GitHub

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

Jump to bottom

[FIRRTL] Performance Degradation in Dedup Runtime between Chisel3.6 and Chisel6 #6979

Closed
Heaton15 opened this issue May 1, 2024 · 0 comments · Fixed by #6985
Closed

[FIRRTL] Performance Degradation in Dedup Runtime between Chisel3.6 and Chisel6 #6979

Heaton15 opened this issue May 1, 2024 · 0 comments · Fixed by #6985

Comments

@Heaton15
Copy link

Heaton15 commented May 1, 2024

When running de-duplication on a .fir file with several ChipTops, the dedup process increases by 6-10x.
For chisel3.6 dedup time is 9.2838 ( 3.0%) 9.2838 ( 9.6%) Dedup.
For chisel6 dedup time is 63.3732 ( 17.5%) 63.3732 ( 41.8%) Dedup

You can recreate this,at least in chipyard, by building the following config on different versions of chisel with
make -C sims/vcs CONFIG=RocketMultiChipConfig

Example Multi ChipTop Config

class RocketMultiChipConfig
    extends Config(
      new chipyard.harness.WithHomogeneousMultiChip(
        n = 32,
        new Config(
          new RocketConfig
        )
      ) ++
        new chipyard.harness.WithAbsoluteFreqHarnessClockInstantiator
    )

While doing a lot of printf debugging, I found that the increased time is spent in this section of the code, which seems to be just processing the annotation: https://github.com/llvm/circt/blob/main/lib/Dialect/FIRRTL/Transforms/Dedup.cpp#L1074-L1118

Some observations I also encountered were that deleting all of the DedupGroupAnnotations in the .fir dropped the dedup time substantially (which I suppose would be an obvious solution since previous versions of chisel didn't use these annotations).

In a custom Config that uses WithHomogeneousMultiChip , I have built 4 ChipTops and also seen the total memory balloon to 100GB of RAM during the entire dedup process.

Workaround in the meantime is to just add the --no-dedup flag.
dtzSiFive added a commit to dtzSiFive/circt that referenced this issue May 3, 2024
@dtzSiFive
[FIRRTL][Dedup] Alter dedup group handling to avoid exponential behav…
c0f7e27 
…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.
dtzSiFive added a commit to dtzSiFive/circt that referenced this issue May 3, 2024
@dtzSiFive
[FIRRTL][Dedup] Alter dedup group handling to avoid exponential behav…
71f38fe 
…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.
dtzSiFive added a commit to dtzSiFive/circt that referenced this issue May 3, 2024
@dtzSiFive
[FIRRTL][Dedup] Alter dedup group handling to avoid exponential behav…
1db437c 
…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.
@dtzSiFive dtzSiFive mentioned this issue May 3, 2024
Merged
dtzSiFive added a commit that referenced this issue May 3, 2024
@dtzSiFive
[FIRRTL][Dedup] Alter dedup group handling, avoid exponential behavio…
62fbb3a 
…r. (#6985)

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.

Also:
Cleanup all module-like's, not just fmoduleop's.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[FIRRTL][Dedup] Alter dedup group handling, avoid exponential behavior. dtzSiFive/circt
1 participant
@Heaton15