Add opt_addcmp: fuse an adder into the comparator it feeds - #307
Merged
Conversation
Author
|
@greptileai review |
Greptile SummaryThe PR adds an unsigned, non-truncating add-compare fusion pass and extracts mux_push’s unit-delay analysis into a shared helper.
Confidence Score: 5/5The PR appears safe to merge; no concrete correctness, build, security, or selection-boundary failure was identified. The rewrite checks unsignedness, mathematical-sum width, complete comparator consumption, sharing, and timing eligibility, while the timing extraction preserves mux_push’s existing implementation and the tests exercise the principal semantic and structural boundaries.
|
| Filename | Overview |
|---|---|
| passes/opt/opt_addcmp.cc | Adds guarded recognition and carry-save replacement for unsigned, non-truncating add-compare regions; no actionable defect was established. |
| passes/silimate/unit_delay.h | Extracts the existing iterative unit-delay arrival/departure model into a reusable worker base. |
| passes/silimate/mux_push.cc | Replaces the local timing implementation with inheritance from the character-equivalent shared helper. |
| tests/silimate/opt_addcmp.ys | Covers nominal equivalence, mirrored relations, width variation, structural results, and guarded rejection cases. |
| passes/opt/CMakeLists.txt | Registers the new optimization pass in the CMake component list. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Unsigned non-truncating add] --> B[Relational comparator]
B --> C{Sum shared?}
C -->|No| D[Apply fusion]
C -->|Yes| E{Timing mode and critical path?}
E -->|Yes| D
E -->|No| F[Keep original structure]
D --> G[Carry-save XOR/majority level]
G --> H[Single relational comparator]
Reviews (1): Last reviewed commit: "Add opt_addcmp: fuse an adder into the c..." | Re-trigger Greptile
akashlevy
force-pushed
the
qor/cmp-onehot
branch
from
August 28, 2026 02:12
1ac4417 to
3304f86
Compare
A bound check spelled `a + b <cmp> c` asks for the order of a sum, never
for the sum, but the RTL puts a full carry-propagate adder in front of the
comparator's own carry chain. opt_addcmp replaces the adder with one
carry-save level, so only the comparator's chain is left on the path:
(a + b) >= c -> s >= ~(v << 1) s = a ^ b ^ ~c, v = maj(a, b, ~c)
Operands are widened to max(|a|,|b|,|c|) + 1 first, which makes the
identity exact rather than modular. A truncating add (comparing a residue)
and any signed operand are rejected. All four relations and either operand
order are handled by picking a relation and an inversion.
When the comparator is the sum's only reader the adder dies with the
rewrite, so it always fires. When the sum has other readers the adder stays
and the carry-save level is added area, which only pays on a critical
comparator, so that case is behind -timing.
Nothing invokes the pass yet, so this is a no-op on its own; the Preqorsor
side adds it to the matcher block.
Also lifts mux_push's unit-delay arrival/departure model into
passes/silimate/unit_delay.h rather than making a fourth copy of it: the
-timing guard has to agree with mux_push's on which paths are critical.
mux_push now derives from it, which is a pure refactor (its three test
scripts and the Preqorsor qor_* suite are unchanged).
Co-authored-by: Cursor <cursoragent@cursor.com>
The unit-delay model memoized per bit, but every output bit of a cell shares
one arrival and every input bit one departure, so it recomputed the same number
once per bit and rescanned the cell's whole port list each time: O(width**2) per
cell. Key the caches on the cell instead. On a 800-cell chain of 512-bit
operators, computing the module depth drops from 6.2s to under 0.05s, and the
cost stops growing with operand width. mux_push's four cache-clearing blocks
become invalidate_timing(), since there are now caches it did not know about.
The values are unchanged by construction, and 66 netlists across six
timing-guarded muxpush/opt_addcmp configurations come out byte-identical.
opt_addcmp also skips modules with no comparator before indexing every bit in
them, and now covers two more shapes:
- $eq/$ne, off the same pair the ordering relations use: a + b == c iff
s + d == 2**W-1, and two values summing to all-ones share no set bit, so
that is just s == ~d.
- add trees, flattened to n summands and reduced back to two by run_csa().
Running at max(|o_i|) + ceil(log2 n) bits keeps the total below 2**W, which
is what makes every carry shift exact. Only a child add its parent solely
reads is absorbed, so each adder taken in is dead afterwards.
$sub stays out: no width condition makes an unsigned subtract exact the way one
does an add, since a - b wraps whenever a < b.
Tests add SAT equivalence for equality and for three- and four-summand trees,
plus negatives for a shared child, a truncating child, and $sub.
Co-authored-by: Cursor <cursoragent@cursor.com>
akashlevy
force-pushed
the
qor/cmp-onehot
branch
from
August 28, 2026 02:49
3304f86 to
ab729a7
Compare
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
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.
What
A bound check spelled
a + b <cmp> casks for the order of a sum, never forthe sum itself, but the RTL puts a full carry-propagate adder in front of the
comparator's own carry chain.
opt_addcmpreduces the three operands to a sumand a carry vector in constant depth, leaving one carry chain instead of two:
Operands are widened to
max(|a|,|b|,|c|) + 1first. That is what makes theidentity exact rather than modular: the carry out of the top column is
maj(0, 0, x) = 0, so the shift drops nothing. Derivation, at widthWwitha + b < 2**W:The strict form drops the
+1, turning>=into>. The other two relationsare these two inverted, and a sum on the comparator's right-hand side is the
mirror image, so all four cell types are covered by choosing a relation and an
inversion.
Soundness conditions, both structural:
Ymust be at leastmax(|a|,|b|) + 1bits andthe comparator must read all of it. An add that wraps compares a residue,
which the carry-save form does not reproduce.
Profitability: when the comparator is the sum's only reader the adder is dead
after the rewrite, so it is a strict win and always taken. When the sum has
other readers the adder stays and the carry-save level is added area, which
only pays on a critical comparator — that case is behind
-timing.Shared timing model
The
-timingguard has to agree withmuxpush's on which paths are critical,so rather than adding a fourth copy of the unit-delay heuristic this lifts
mux_push's arrival/departure model intopasses/silimate/unit_delay.handderives both workers from it. That half is a pure refactor:
mux_push.ccloses202 lines and gains 3, and the extracted bodies are character-identical modulo
indentation. (
opt_timing_balanceandopt_carry_selectkeep their ownvariants — those cost a cell in fractional levels against its output width,
which is a different currency.)
Landing order
Nothing invokes
opt_addcmp, so this PR is a no-op on its own and safe to mergefirst. Silimate/preqorsor adds it to the matcher block in a follow-up, which is
where the behavior change lands atomically.
Timing model scaling
The extracted model memoized per bit, but every output bit of a cell shares one
arrival (the max over all its inputs) and every input bit shares one departure,
so it recomputed the same number once per bit and rescanned the cell's whole
port list each time -- O(width**2) per cell, on a model that exists to be
consulted from wide datapaths. The caches are now keyed on the cell.
Isolated cost of
compute_module_depth()on a chain of 800 operators, sweepingtheir width (
opt_addcmp -timingwith a shared, off-critical bound check, sothe guard runs but nothing is rewritten):
Before quadruples with each doubling of width; after is flat in it.
The values are unchanged by construction -- the per-bit recurrence and the
per-cell one compute the same max -- and 66 netlists across six timing-guarded
muxpush/opt_addcmpconfigurations and 11 designs come out byte-identical.#315 landed the same factoring from the other side -- a lazy
longest_path()and a
reset_timing()helper -- so both now live inUnitDelayTimingandopt_addcmppicks up the lazy depth too. The two changes are complementary:#315 stopped rebuilding the model per candidate, this one stops the model
itself from costing O(width**2).
opt_addcmpalso checks a module has a comparator at all before indexing everybit in it.
Generalization
$eq/$ne, off the same pair the ordering relations already build:a + b == ciffs + d == 2**W-1, and two values summing to all-ones canshare no set bit (it would carry and clear one), so that is exactly
s == ~d.run_csa()reduces n summands back to the two the identityexpects. Working at
max(|o_i|) + ceil(log2 n)bits keeps the running totalbelow
2**W, so every value in the tree is too, which is what makes eachcarry shift exact rather than modular. Only a child add its parent solely
reads is absorbed, so every adder the walk takes in is dead afterwards; a
truncating child stays an operand, since its wrapped result is not the sum of
its own operands.
$subdeliberately stays out. No width condition makes an unsignedsubtract exact the way one does an add:
a - bwraps whenevera < b, andruling that out needs a value range the pass cannot establish locally.
New tests: SAT equivalence for equality and disequality on both sides, SAT
equivalence for three- and four-summand trees, a structural check that a
three-summand tree leaves both adders dead, and negatives for a shared child, a
truncating child, and
$sub.qor_cmp_onehot_path12is byte-identical before and after this second commit(its inner
pid + off_qtruncates, so it is correctly left as an operand), andequiv_opt -assertstill proves the fusion on it.Test plan
tests/silimate/opt_addcmp.ys(auto-discovered bygenerate_mk.py) covers:$addgone, one$geandtwo
$xorleft)-timing-min-widthmux_push.ys,mux_push_types.ys,mux_push_farm_gain.ysandopt_vps.yspass unchanged against the refactor.
Downstream, on Preqorsor's
qor_cmp_onehot_path12(address bound check feedinga one-hot decode): lol 24.25 -> 20.75, clk 358 -> 324, area 39.3 -> 45.4. An
A/B over 15 mixed designs (compares, adders, carry-save, barrel shifters, ibex)
leaves the other 14 identical in lol and clk. Each rewrite in that run was also
proved by
equiv_opt -assertin Preqorsor's formal mode.Made with Cursor