opt_balance_tree: -sliced-profit keeps a nest the tree would deepen - #300
opt_balance_tree: -sliced-profit keeps a nest the tree would deepen#300akashlevy wants to merge 1 commit into
Conversation
The sliced-$add matcher flattens a nest of adders into summands and
rebuilds it as a balanced tree, which pays when those summands were
already crossing adders. It also splits out leaf slices that the nest
merely concatenated into one operand: those cost nothing where they are
and a full-width adder each in the tree.
A conditional {x[14:0], c} added to x is one 18-bit CPA in the nest and
two 17-bit CPAs after flattening, because the 1-bit c is read as a fourth
summand. Under -sliced-profit the matcher compares the worst-case carry
depth (summed adder widths) of both shapes and keeps the nest when the
tree is deeper.
Default-off, so the pass is unchanged without the option.
Co-authored-by: Cursor <cursoragent@cursor.com>
Greptile SummaryThe PR adds an optional profitability guard that compares carry depths before flattening sliced add nests.
Confidence Score: 4/5The PR should not merge until Test 36 is corrected or its input is changed to a genuinely profitable sliced tree. The new guard computes Test 36's balanced tree as deeper than its nest and therefore cannot produce the structure asserted by the added regression test. Files Needing Attention: tests/opt/opt_balance_tree.ys
|
| Filename | Overview |
|---|---|
| passes/opt/opt_balance_tree.cc | Adds the default-off carry-depth profitability guard; its recurrence consistently models the emitted balanced tree. |
| tests/opt/opt_balance_tree.ys | Adds two regression cases, but Test 36 expects flattening even though its tree computes deeper than its nest. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Extract sliced add summands] --> B[Compute maximum nested carry depth]
A --> C[Compute balanced-tree carry depth]
B --> D{Tree depth <= nest depth?}
C --> D
D -->|Yes| E[Flatten into balanced tree]
D -->|No| F[Keep sliced nest]
F --> G[Test 36 structural assertion fails]
Reviews (1): Last reviewed commit: "opt_balance_tree: -sliced-profit keeps a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Heads up that the design cited in the test plan no longer justifies this option,
The two guards are close to disjoint, though. #305 refuses a child that drops its module test (
input wire clk,
input wire [7:0] a_in, b_in, input wire [3:0] lo_in, input wire [17:0] c_in,
output reg [17:0] y
);
reg [7:0] a, b; reg [3:0] lo; reg [17:0] c;
always @(posedge clk) begin a <= a_in; b <= b_in; lo <= lo_in; c <= c_in; end
wire [8:0] s = a + b; // 9 bits over 8-bit operands: carry kept
always @(posedge clk) y <= {s, lo} + c;
endmoduleBuilt #305 and this PR separately on current
#305 is bit-identical to pristine, as expected, while Something along those lines would give the option a regression that #305 cannot |
|
Completing the picture from my last comment: I censused which of our regressions
The other 24 never flatten a sliced nest, so neither PR can reach them. Two things worth knowing before merge:
None of that argues for closing -- the mechanism is sound and the residual set is |
|
Closing for now. The mechanism is sound and the residual set is real, but it does
Worth reopening if a real design turns up that flattens a carry-complete nest and |
1 participant
Summary
The sliced-
$addmatcher inopt_balance_treeflattens a nest of adders into aflat summand list and rebuilds it as a balanced tree. That pays when the
summands were already crossing adders, but the extraction also splits out leaf
slices that the nest merely concatenated into one operand. Those slices cost
nothing where they are and a full-width adder each in the tree, so flattening
them adds carry-propagate levels instead of removing them.
Concretely,
x + (c ? {x[14:0], b} : 0)is one 18-bit CPA in the nest. The 1-bitbis the output of a narrow$add, so the matcher reads it as a fourth summandand emits a two-level tree: two 17-bit CPAs in series.
-sliced-profitcompares the worst-case carry depth of both shapes -- the summedwidths of the adders a summand crosses -- and keeps the nest when the tree is
deeper. The estimate mirrors
create_balanced_tree()'s own split, so it scoresthe shape that would actually be emitted.
The option is default-off: without it the pass behaves exactly as before.
Test plan
tests/opt/opt_balance_tree.ystest 35 (new): the concatenated-carry nestflattens into three adds by default and is left alone under
-sliced-profit, withequiv_opt -asserton the guarded run.tests/opt/opt_balance_tree.ystest 36 (new): the existing sliced shiftedADD chain (test 31) still flattens to
add3_tree_3under-sliced-profit.area 55.0 -> 53.7, with the flag off bit-identical to a pristine build.
qor_vmw_add_tree{,_v1..v5}, which exist to exercise this matcher, areunchanged in lol; the guard only declines the redundant second flatten of
an already-built tree.
Made with Cursor