opt_balance_tree: do not reassociate across a discarded carry - #305
Conversation
is_right_type() accepts $add cells that drop their own carry out, but rebuilding a tree from such a cell's operands computes that carry back and lets it reach a bit the original circuit never saw. Three lines of Verilog miscompile today, and a fuzz of small add networks hits it in 25 of 400 designs. Follow a carry-dropping link only where the discarded bit stays dead: a consumer that truncates at least as hard in the plain chain matcher, and a carry position at or above the head's output width in the sliced one. A cell whose only continuation is a rejected link now heads its own tree, so the subchain below the cut is still balanced. Co-authored-by: Cursor <cursoragent@cursor.com>
Greptile SummaryThe PR prevents
Confidence Score: 5/5The PR appears safe to merge; the new guards preserve modular-addition boundaries while retaining balancing where discarded bits remain unobservable. The plain matcher cuts widening links after a truncated add, the sliced matcher compares the restored carry position against the current head width, and the added tests exercise both rejected and permitted boundary cases without leaving a concrete blocking failure.
|
| Filename | Overview |
|---|---|
| passes/opt/opt_balance_tree.cc | Adds correctly bounded carry-truncation guards for plain and sliced add-tree extraction, plus independent subchain-head recovery. |
| tests/opt/opt_balance_tree.ys | Adds focused equivalence and topology regressions for unsafe widening, safe modular chains, sliced carry boundaries, and stranded-subchain recovery. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Candidate child add] --> B{Child discards carry?}
B -->|No| C[Allow chain traversal]
B -->|Yes| D{Plain or sliced link?}
D -->|Plain| E{Consumer width <= child width?}
E -->|Yes| C
E -->|No| F[Cut link and balance child subchain separately]
D -->|Sliced| G{Carry position >= head width?}
G -->|Yes| C
G -->|No| F
Reviews (1): Last reviewed commit: "opt_balance_tree: do not reassociate acr..." | Re-trigger Greptile
Problem
opt_balance_treerebalances$addchains, andis_right_type()deliberatelyaccepts adders that discard their own carry out:
Accepting those cells as chain members is not sound. Flattening
a + binto thesummand list of a wider consumer drops the truncation that adder applied, so the
rebuilt tree computes the discarded carry back and lets it reach a bit the
original circuit never saw. Three lines are enough to miscompile:
equiv_opt -assert opt_balance_treeleaves 8 of 16 output bits unproven on thisdesign today. A random fuzz of small add networks (2-5 adders, widths 2-8, mixed
zero extension and slicing) miscompiles 25 of 400 designs.
Both matchers are affected, for the same reason but through different code:
head's truncation point;
is the shape you get without any concatenation, since Verilog leaves the width
extension to the sink's port parameters.
Fix
Keep the aggressive matching, but only follow a carry-dropping link when the
discarded bit is provably still dead afterwards:
chain_driver()refuses a carry-dropping driver whose consumer is wider.Equal or narrower is fine: mod-2^n addition is associative, and mod-2^n
followed by mod-2^m with m <= n is just mod-2^m.
base_offset + pos + widthsits below the head's output width, i.e. lands ona bit the head still keeps.
Cutting a chain that way would also strand the truncating adder's own subchain,
since head detection would never treat it as a chain end. So a cell whose only
continuation is a link the guard rejects now heads its own tree, and the
subchain below it still gets balanced.
Validation
tests/opt/opt_balance_tree.ysgains 6 cases (35-40): the plain and slicedmiscompiles above, a carry-dropping child whose carry does fall off the head's
truncation (must still balance), a same-width
a+b+c+dchain (must stillbalance), and the stranded-subchain recovery.
mainand pass here; the other 34 are unchanged.Test 36 also fails on a build carrying only the two guards, so the
head-detection half is covered too.
tests/opt/opt_addcin.ys,tests/opt/opt_parallel_prefix.ysand the wholetests/arith_tree/suite pass.main.QoR
A/B over 58 Preqorsor designs (add trees, adder chains, compares, barrel
shifters, multipliers, priority encoders, plus non-arithmetic controls). 53 come
out bit-identical.
clkis delay in ps, so lower is better:qor_pow2_round_sizeis the honest cost of the guard. Its 6-bitsliceaddertruncates, and folding it into the two 7-bit exponent adders reading it is in
fact equivalent -- SAT proves it on the real netlist -- but only because of a
value range the pass cannot establish locally, so the guard conservatively
refuses. Extending the guard to discount constant-zero operand padding was
tried and recovers nothing here, so it is not part of this change.
Its LoL bound is raised in
Silimate/preqorsor#2292,
which should merge before this ships in a wheel.