llvm / llvm/circt

[FIRRTL] IMConstProp (and others) should preserve InstanceGraph

Open
#2,605 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

FIRRTL
Dominant language
C++
Stars
2.2k
Forks
524
Avg merge
3d 2h
Merged PRs (30d)
46

Description

Seems like we are not preserving the `InstanceGraph` analysis in `IMConstProp`.
We have to look at all the passes that are using the `InstanceGraph` analysis, and if it can be preserved.
In the following runtime output, any pass with `(A) circt::firrtl::InstanceGraph` is redoing the analysis.
```
----User Time---- ----Wall Time---- ----Name----
35.7049 ( 6.6%) 35.7049 ( 13.3%) FIR Parser
466.8056 ( 86.6%) 195.7462 ( 73.1%) 'firrtl.circuit' Pipeline
60.0645 ( 11.1%) 3.6523 ( 1.4%) 'firrtl.module' Pipeline
55.9135 ( 10.4%) 3.4582 ( 1.3%) CSE
0.1137 ( 0.0%) 0.0164 ( 0.0%) (A) DominanceInfo
4.0435 ( 0.8%) 0.3411 ( 0.1%) LowerCHIRRTLPass
13.9256 ( 2.6%) 13.9256 ( 5.2%) InferWidths
15.9592 ( 3.0%) 15.9592 ( 6.0%) InferResets
0.7073 ( 0.1%) 0.7073 ( 0.3%) (A) circt::firrtl::InstanceGraph
4.8849 ( 0.9%) 4.8849 ( 1.8%) WireDFT
10.3873 ( 1.9%) 10.3873 ( 3.9%) PrefixModules
18.9497 ( 3.5%) 18.9497 ( 7.1%) LowerFIRRTLTypes
172.5599 ( 32.0%) 15.7106 ( 5.9%) 'firrtl.module' Pipeline
28.5536 ( 5.3%) 1.9058 ( 0.7%) ExpandWhens
0.3131 ( 0.1%) 0.1318 ( 0.0%) RemoveResets
132.6794 ( 24.6%) 12.9047 ( 4.8%) Canonicalizer
10.9484 ( 2.0%) 0.8854 ( 0.3%) InferReadWrite
13.3058 ( 2.5%) 13.3058 ( 5.0%) Inliner
14.8439 ( 2.8%) 14.8439 ( 5.5%) IMConstProp
0.5209 ( 0.1%) 0.5209 ( 0.2%) (A) circt::firrtl::InstanceGraph
7.0860 ( 1.3%) 7.0860 ( 2.6%) BlackBoxReader
0.5011 ( 0.1%) 0.5011 ( 0.2%) (A) circt::firrtl::InstanceGraph
47.1971 ( 8.8%) 47.1971 ( 17.6%) GrandCentral
6.7771 ( 1.3%) 6.7771 ( 2.5%) GrandCentralTaps
0.5238 ( 0.1%) 0.5238 ( 0.2%) (A) circt::firrtl::InstanceGraph
43.7669 ( 8.1%) 7.1336 ( 2.7%) 'firrtl.module' Pipeline
0.1425 ( 0.0%) 0.0246 ( 0.0%) GrandCentralSignalMappings
43.3741 ( 8.0%) 7.1308 ( 2.7%) Canonicalizer
6.0484 ( 1.1%) 6.0484 ( 2.3%) RemoveUnusedPorts
0.4631 ( 0.1%) 0.4631 ( 0.2%) (A) circt::firrtl::InstanceGraph
3.6972 ( 0.7%) 3.6972 ( 1.4%) CreateSiFiveMetadata
0.4078 ( 0.1%) 0.4078 ( 0.2%) (A) circt::firrtl::InstanceGraph
6.1606 ( 1.1%) 6.1606 ( 2.3%) EmitOMIR
```

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by tracing the FIRRTL passes named in the runtime output, especially IMConstProp, BlackBoxReader, GrandCentral, RemoveUnusedPorts, and CreateSiFiveMetadata, and inspect their use of the InstanceGraph analysis. Compare which passes can preserve that analysis after running. Done means the applicable passes preserve InstanceGraph and the runtime output no longer shows avoidable recomputation.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers, performance
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.