opt_compact_prefix tests: scope the self-equivalence proofs to gate - #311
Merged
Conversation
All five SAT self-equivalence arms invoked `opt_compact_prefix` with no selection argument at a point where `gold` already existed, so the pass optimized `gold` too and each miter compared optimized against optimized. Every solve ran on 2 variables / 4 clauses; scoped correctly they run on 1796-6405. Co-authored-by: Cursor <cursoragent@cursor.com>
Greptile SummaryThe PR repairs five previously vacuous
Confidence Score: 5/5The PR appears safe to merge; the updated scripts now compare the preserved reference modules against independently optimized gate modules. The scoped pass argument selects each renamed
|
| Filename | Overview |
|---|---|
| tests/silimate/opt_compact_prefix.ys | Correctly scopes optimization to gate in all five self-equivalence arms while restoring full selection before shared cleanup and miter construction. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
SRC[Import source] --> GOLD[Rename first module to gold]
SRC --> GATE[Import again and rename to gate]
GATE --> PASS[opt_compact_prefix gate]
PASS --> CLEAR[select -clear and opt_clean]
GOLD --> MITER[Equivalence miter]
CLEAR --> MITER
MITER --> SAT[SAT proof]
Reviews (1): Last reviewed commit: "opt_compact_prefix tests: scope the self..." | Re-trigger Greptile
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.
Sibling of #310, same flaw in a different test file, found by auditing the whole
test suite for the idiom that #310 fixed.
The idiom
A Yosys pass with no selection argument runs on every module in the design. These
tests build
goldfirst andgatesecond:By the time
opt_compact_prefixruns,goldis live, so the bare invocation optimizesit as well and the miter compares optimized against optimized.
goldandgateend upbyte-identical, and the proof is not merely weakened but vacuous.
Order is what matters: an arm that builds
gatebeforegoldexists is unaffected.Affected
All five SAT self-equivalence arms in
tests/silimate/opt_compact_prefix.ys:All five still prove
SUCCESS, soopt_compact_prefixis genuinely correct on thesecases — the tests were the only thing broken.
These arms were worthless, not just weak
Injecting a gross miscompile into arm 1 (
chtype -map $add $sub, i.e. every adderbecomes a subtractor) and running it the way a buggy pass would run, bare:
Solving problem with 2 variables and 4 clauses->no model found: SUCCESS!, exit 0. The corruption hitsgoldtoo, so the miter cannot see it.Solving problem with 2226 variables and 6059 clauses->model found: FAIL!, exit 1.The same was independently demonstrated for
opt_vpsin #310, where a version of thepass that provably left four output bits undriven still passed a vacuous arm.
Fix
Move
rename ... gateahead of the pass and scope the pass togate, exactly as #310does:
select -clearis needed because handing a pass a selection argument overwrites thecurrent selection, and the following
opt_cleanshould still see the whole design.The two modulo-decimation arms keep
bmuxmapunscoped:satneeds$bmuxlowered onboth arms, and it is a semantics-preserving lowering applied symmetrically, exactly like
the trailing bare
opt_clean.Audit scope
I applied the same detector to the whole
tests/tree. Withintests/silimatethis filewas the only remaining instance; everything else is safe, by four distinct mechanisms:
opt_argmax.ys(7 arms),opt_priority_onehot.ys(8 arms) — already scoped withselect -module X/select -clear. Solves run on 2315-14525 and 1839-6732 variables.opt_carry_select.ys(2 arms) — pass already given an explicit module selection. 964 and 1912 variables.opt_boundary.ys(16 arms),opt_boundary_random.tcl,mux_push.ys(1equiv_makearm) — snapshot idiom (design -save/-load/-stash), so the reference is not a live module and a bare pass cannot reach it. Structurally immune.carvenetlist.ys(6 arms) — thegoldenreference is a hand-written module read in after the pass runs.equiv_opt, which handles gold/gate internally and is unaffected.Outside
tests/silimate, thecd-scoped comparisons intests/opt/opt_expr_*.ys,tests/opt/bug1758.ys,tests/fsm/uut_*.ysandtests/sat/splice.ysare all correctlyscoped, and
tests/verific/case.ysre-runsprepongoldbut is not vacuous (its twoarms come from different Verific configurations; solves run on 180 and 348 variables).
Testing
Full
tests/silimatesuite: 40 PASS, 1 FAIL. The failure ismux_push.ys, pre-existingand unrelated (a
t:$shrcell-count assertion from in-flight work onmux_push.cc).Made with Cursor