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

[LowerFirReg] Reimplement the mux reachability analysis #6709

Merged
merged 18 commits into from
Apr 9, 2024

Conversation

prithayan
Copy link
Contributor

The PR implements a new heuristic to determine if a Mux is reachable from a FirReg.
In general an operation is reachable from a register if its in the fanout of the register.
For FirReg lowering, an if/else structure is required for proper enable inference, if a mux is within the fanout from the register.
The fanout path can only consist of MuxOp, ArrayGetOp or ArrayCreateOp.
Thus any ops other than MuxOp, ArrayGetOp or ArrayCreateOp block the reachability.
The analysis is built lazily when its queried and the result is cached to avoid redundant traversal of the IR.

Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

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

I left some minor comments. In general, this iteration seems to be heading in a great direction. Thanks for taking the quick n dirty solution I put together and thinking hard about how to do it both correctly and efficiently.

lib/Conversion/SeqToSV/FirRegLowering.h Outdated Show resolved Hide resolved
lib/Conversion/SeqToSV/FirRegLowering.h Outdated Show resolved Hide resolved
lib/Conversion/SeqToSV/FirRegLowering.h Outdated Show resolved Hide resolved
lib/Conversion/SeqToSV/FirRegLowering.h Outdated Show resolved Hide resolved
lib/Conversion/SeqToSV/FirRegLowering.h Show resolved Hide resolved
Copy link
Contributor

@dtzSiFive dtzSiFive left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this! Left some feedback 👍 .

lib/Conversion/SeqToSV/FirRegLowering.cpp Outdated Show resolved Hide resolved
lib/Conversion/SeqToSV/FirRegLowering.cpp Outdated Show resolved Hide resolved
lib/Conversion/SeqToSV/FirRegLowering.cpp Outdated Show resolved Hide resolved
lib/Conversion/SeqToSV/FirRegLowering.h Outdated Show resolved Hide resolved
lib/Conversion/SeqToSV/FirRegLowering.h Outdated Show resolved Hide resolved
lib/Conversion/SeqToSV/FirRegLowering.cpp Outdated Show resolved Hide resolved
lib/Conversion/SeqToSV/FirRegLowering.cpp Outdated Show resolved Hide resolved
lib/Conversion/SeqToSV/FirRegLowering.cpp Show resolved Hide resolved
lib/Conversion/SeqToSV/FirRegLowering.cpp Show resolved Hide resolved
Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

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

I think the core logic here is good, and certainly better than what we had. Please look through Will's open comments, but I think we can merge this.

@prithayan prithayan force-pushed the dev/pbarua/improve-lower-seq-to-sv branch 2 times, most recently from 1643010 to 950b338 Compare March 8, 2024 03:32
@prithayan prithayan force-pushed the dev/pbarua/improve-lower-seq-to-sv branch from 950b338 to 12a9689 Compare April 9, 2024 20:01
@prithayan prithayan merged commit ac8f776 into main Apr 9, 2024
4 checks passed
@prithayan prithayan deleted the dev/pbarua/improve-lower-seq-to-sv branch April 9, 2024 21:30
@prithayan
Copy link
Contributor Author

Thanks for all the feedback. Finally merged this PR and sorry for the delay, I just realized it was still open.
Testing on some internal cores, LowerSeqToSV time reduced from 1.2% to 0.5% of the total runtime.

prithayan added a commit that referenced this pull request Apr 9, 2024
@prithayan prithayan restored the dev/pbarua/improve-lower-seq-to-sv branch April 9, 2024 23:45
prithayan added a commit that referenced this pull request Apr 11, 2024
This implements a new heuristic to determine if a Mux is reachable from a
FirReg. In general an operation is reachable from a register if its in the
fanout of the register. For FirReg lowering, an if/else structure is required
for proper enable inference, if a mux is within the fanout from the register.
The fanout path can only consist of MuxOp, ArrayGetOp or ArrayCreateOp.
Thus any ops other than MuxOp, ArrayGetOp or ArrayCreateOp block the
reachability. The analysis is built lazily when its queried and the result is
cached to avoid redundant traversal of the IR.
This fixes an issue that caused the merged commit for #6709 to be reverted.
The was to explicitly specify the default size of `SmallVector`.
cepheus69 pushed a commit to cepheus69/circt that referenced this pull request Apr 22, 2024
The PR implements a new heuristic to determine if a Mux is reachable from a
 FirReg. In general an operation is reachable from a register if its in the
 fanout of the register. For FirReg lowering, an if/else structure is required
 for proper enable inference, if a mux is within the fanout from the register.
The fanout path can only consist of MuxOp, ArrayGetOp or ArrayCreateOp.
Thus any ops other than MuxOp, ArrayGetOp or ArrayCreateOp block the
 reachability. The analysis is built lazily when its queried and the result is
 cached to avoid redundant traversal of the IR.
cepheus69 pushed a commit to cepheus69/circt that referenced this pull request Apr 22, 2024
cepheus69 pushed a commit to cepheus69/circt that referenced this pull request Apr 22, 2024
This implements a new heuristic to determine if a Mux is reachable from a
FirReg. In general an operation is reachable from a register if its in the
fanout of the register. For FirReg lowering, an if/else structure is required
for proper enable inference, if a mux is within the fanout from the register.
The fanout path can only consist of MuxOp, ArrayGetOp or ArrayCreateOp.
Thus any ops other than MuxOp, ArrayGetOp or ArrayCreateOp block the
reachability. The analysis is built lazily when its queried and the result is
cached to avoid redundant traversal of the IR.
This fixes an issue that caused the merged commit for llvm#6709 to be reverted.
The was to explicitly specify the default size of `SmallVector`.
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.

4 participants