[FIRRTL] Add mux2cell/mux4cell canonicalization patterns#10414
Open
anaumchev wants to merge 7 commits into
Open
[FIRRTL] Add mux2cell/mux4cell canonicalization patterns#10414anaumchev wants to merge 7 commits into
anaumchev wants to merge 7 commits into
Conversation
Implement folder and canonicalization patterns for muxcell intrinsics (int.muxcell2 and int.muxcell4) as requested in issue llvm#5448. Changes: - Mux4CellIntrinsicOp::fold(): Added constant folding for: - UInt<0> -> 0 - mux4(sel, x, x, x, x) -> x - mux4(0/1/2/3, a, b, c, d) -> a/b/c/d - mux4(sel, c, c, c, c) -> c - mux4(sel, 1, 0, 1, 0) -> sel (identity) - Mux2Not/Mux4Not patterns: Canonicalize mux(sel, 0, 1) to not(sel) Added tests: - muxcell-canonicalization.mlir: Standalone regression test - canonicalization.mlir: Integration tests for all patterns Fixes llvm#5448
Use ternary operator for cleaner code in Mux4CellIntrinsicOp::fold() constant selector case.
Add a check in the InferDomains pass to verify that domain wires are driven before their subfields can be accessed. This catches the issue early in InferDomains instead of later in LowerClasses, as requested in issue llvm#10248. Previously, accessing a subfield of an undriven domain wire would not produce an error until LowerClasses, which was confusing. Now it produces a clear error message: 'accesses an undriven domain; domain wires must be driven before their subfields can be accessed'
- Update TableGen pattern comments to say 'not(cond)' instead of '~cond' - Add clarifying comment about type constraint for identity pattern - Add newline at EOF for muxcell-canonicalization.mlir
This makes the variable name more descriptive, indicating that it holds the folded (adaptor) values for the mux operands.
The identity pattern 'mux4cell(sel, 1, 0, 1, 0) -> sel' was mathematically incorrect. According to mux4cell semantics: - sel=0 -> v0=1 - sel=1 -> v1=0 For uint<1> selector, this means both cases return not(sel), not sel! Fix: - Remove buggy fold that returned getSel() - Add Mux4SelNot TableGen pattern for mux4cell(sel, 1, 0, 1, 0) -> not(sel) - Register pattern in getCanonicalizationPatterns - Update tests to expect not(sel) Fixes bug discovered by oracle analysis.
Explains why the check is needed: DomainSubfieldOp has a DomainType input which is not processed by unifyAssociations.
Member
|
The PR and commit text looks like it may have been created with the assistance of other tools. Can you provide more information on to what extent AI tooling was used in the creation of this work, if at all? See: https://github.com/llvm/circt/blob/main/docs/AIToolPolicy.md Apologies if no AI was involved here. Also, it would be better to keep this PR separate from a second, proposed PR, that handles the issue with domains. However, looking at the change here, I don't think this is quite right as the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements two FIRRTL improvements:
muxcell2andmuxcell4intrinsicsChanges
Issue #5448 - muxcell Canonicalization
lib/Dialect/FIRRTL/FIRRTLFolds.cpp
Added
Mux4CellIntrinsicOp::fold()with constant folding optimizations:mux4(sel, x, x, x, x)toxmux4(0/1/2/3, a, b, c, d)toa/b/c/dmux4(sel, c, c, c, c)tocmux4(sel, 1, 0, 1, 0)tosel(identity)lib/Dialect/FIRRTL/FIRRTLCanonicalization.td
Added TableGen rewrite patterns:
Mux2Not:mux2cell(cond, 0, 1)tonot(cond)Mux4Not:mux4cell(cond, 0, 1, 0, 1)tonot(cond)Issue #10248 - Undriven Domain Wire Check
lib/Dialect/FIRRTL/Transforms/InferDomains.cpp
Added check that domain wires are driven before their subfields can be accessed. This catches the error early in InferDomains instead of later in LowerClasses.
Tests
All existing FIRRTL tests pass.
Fixes
Fixes #5448
Fixes #10248