opt_shift -expand: do not distribute $add/$sub over a right shift - #301
opt_shift -expand: do not distribute $add/$sub over a right shift#301akashlevy wants to merge 1 commit into
Conversation
The expand_shifts pattern rewrote (a OP b) >> c into (a >> c) OP (b >> c) for every OP it matched, including $add and $sub. That is only valid for a left shift, which is a multiply by 2^c and distributes mod 2^width. A right shift discards a low window the carry crosses, so shifting each operand first throws the carry away: (255 + 1) >> 8 is 1, while (255 >> 8) + (1 >> 8) is 0. Restrict $add/$sub to $shl/$sshl and leave the bitwise ops on all four shift types, where distribution is sound in both directions. The existing test covered SHL across all five operators but SHR across the bitwise ones only, so its equiv_opt -assert never saw the broken combinations. Add the four missing negatives plus the shape this was found on, a shifted increment of a widened operand. Co-authored-by: Cursor <cursoragent@cursor.com>
Greptile SummaryThis PR prevents
Confidence Score: 5/5The PR appears safe to merge, with no actionable defects identified in the changed behavior. The new filter precisely excludes arithmetic expansion across right shifts, and the added tests retain both the arithmetic and shift cells while checking equivalence for the affected cases.
|
| Filename | Overview |
|---|---|
| passes/silimate/peepopt_expand_shifts.pmg | Adds a focused match filter that blocks the four unsound arithmetic right-shift combinations while preserving bitwise and left-shift expansion. |
| passes/silimate/opt_shift.cc | Extends user-facing pass documentation with the carry-loss rationale and a concrete counterexample. |
| tests/silimate/opt_expand_shifts.ys | Adds equivalence and post-optimization shape checks covering unsigned and signed right-shift arithmetic cases plus the motivating widened-increment shape. |
Reviews (1): Last reviewed commit: "opt_shift -expand: do not distribute $ad..." | Re-trigger Greptile
|
Closing in favour of #299, which was opened two minutes after this one and fixes the same bug plus two more. Both PRs correctly identify that module top(input [7:0] a, input [7:0] b, input [1:0] c, output [15:0] y);
wire [7:0] d = a - b;
assign y = d << c;
endmoduleOn #299 also adds a signedness guard, for a narrow operand that the operator pads with its own signedness while the expanded form hands it to a shift that pads with the shift's. #299 has been rebased onto current |
|
Superseded by #299 (see comparison above). |
The bug
peepopt_expand_shiftsrewrote(a OP b) >> cinto(a >> c) OP (b >> c)forevery operator it matched, including
$addand$sub. Distribution is onlyvalid for a left shift, which is a multiply by
2^cand so distributes mod2^width. A right shift discards a low window that the carry crosses, andshifting each operand first throws that carry away.
Minimal counterexample, 9-bit operands,
a = 255,b = 1,c = 8:(a + b) >> c256 >> 8= 1(a >> c) + (b >> c)0 + 0= 0This is reachable from ordinary RTL. A shifted saturating decrement,
({3'd0, w} + 17'd1) >> lv, hits it directly, and Preqorsor runsopt_shift -combine -expandin the default synthesis flow, where theper-pass equivalence check is only active under
options.formal. On thedesign this was found on, Preqorsor's own formal mode fails with 14 unproven
$equivcells before this change and proves equivalence after it.The fix
One
filterline in the pattern:$add/$subonly match$shl/$sshl.Bitwise operators keep all four shift types, including the sign fill of
$sshr, where the fill ofa OP bis the sameOPof the two fills.Exhaustively SAT-checked all 5 operators x 4 shift types with
miter -equiv+sat -prove-asserts, before and after:<<>><<<>>>&|^+-After the fix the 16 sound combinations still fire and still prove equivalent;
the 4 unsound ones no longer fire.
Test coverage gap this closes
tests/silimate/opt_expand_shifts.ysalready usedequiv_opt -assert, but itcovered SHL across all five operators and SHR across the bitwise ones only,
so the broken combinations were never exercised. This adds the four missing
negatives plus the widened-operand shifted increment the bug was found on. The
new negatives fail on
mainand pass here; the five siblingopt_shifttestfiles (
-combine,-descale,-sink,-fuse,-chain) stay green.Blast radius
Scanned all 391 Preqorsor regression designs by running the pass and
classifying every rewrite it reports. Four designs make an unsound rewrite
standalone; in the full synthesis flow only one still does, and it is a new
unregistered case, so no tracked LoL bound moves. The other three
(
qor_descale_edges,qor_mip_pack_barrel,qor_mip_pack_barrel_trunc) comeout bit-identical on
lol/clk/area. Other right-shifted sums in thecorpus (ibex/azadirtl
multdiv_slow, or1200, LU32PEEng) use constant shiftamounts, which fold to bit-slices before the pass sees a
$shr.Removing an invalid rewrite costs depth where it was firing: the affected
design goes from LoL 26.5 to 27.75. That shortcut was a miscompile, and the
sound rewrite for that shape is
opt_shift -descale, which is being extendedseparately.
This is on by default rather than flag-gated: it removes an incorrect
transform, so there is no configuration in which the old behavior is wanted.
Made with Cursor