Skip to content

Conversation

@uenoku
Copy link
Member

@uenoku uenoku commented Oct 13, 2025

Extend the LowerVariadic pass to handle all commutative operations (and, or, xor, mul, add) from the comb dialect, not just AndInverter ops.

The new implementation uses a delay-aware algorithm that builds balanced binary trees by combining values with the earliest arrival times first, minimizing critical path delay. This is implemented with a priority queue that orders values by their arrival time as computed by the IncrementalLongestPathAnalysis.

Co-authored-by: Max Zhou [email protected]

Fix #9066

Extend the LowerVariadic pass to handle all commutative operations (and,
or, xor, mul, add) from the comb dialect, not just AndInverter ops.

The new implementation uses a delay-aware algorithm that builds balanced
binary trees by combining values with the earliest arrival times first,
minimizing critical path delay. This is implemented with a priority queue
that orders values by their arrival time as computed by the
IncrementalLongestPathAnalysis.

Co-authored-by: Max Zhou <[email protected]>
@cowardsa
Copy link
Contributor

Apologies didn't get to this today - will try and look at it in a few hours before I head off for a holiday - but looks really great and will probably get rid of an annoying defeat vs Yosys in the benchmarking

@uenoku uenoku requested a review from fabianschuiki October 13, 2025 20:30
@uenoku
Copy link
Member Author

uenoku commented Oct 13, 2025

Apologies didn't get to this today - will try and look at it in a few hours before I head off for a holiday -

No rush!

Copy link
Contributor

@cowardsa cowardsa left a comment

Choose a reason for hiding this comment

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

LGTM - had one query around lowering ands and ors to trees - but then you had already covered it later on - so please ignore that comment.

Only query would be to check that there is a LEC test (couldn't find one in a brief scan) for potentially a larger variadic lowering test?

Comment on lines +83 to +87
/// Construct a balanced binary tree from a variadic operation using a
/// delay-aware algorithm. This function builds the tree by repeatedly combining
/// the two values with the earliest arrival times, which minimizes the critical
/// path delay.
static LogicalResult replaceWithBalancedTree(
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking - could this also be used in CombToSynth as we perform variadic reduction there too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good points, yes. I think it makes sense to eventually move the utility to Synth header and use it from CombToSynth. Though it might be slightly weird because CombToSynth doesn't have timing info at this point. I'll find a way to unify them.

Comment on lines +9 to +11
// AIG-NEXT: Maximum path delay: 32
// MIG-NEXT: Maximum path delay: 32
// LUT6-NEXT: Maximum path delay: 6
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool to see this test keep getting refined to track progress

@uenoku uenoku merged commit 26a6b6e into llvm:main Oct 14, 2025
7 checks passed
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.

[circt-synth] Comb Canonicalization Unexpectedly Increasing Logic Depth

2 participants