feat: prove correctness of the LLVM-dialect RISC-V combines#1043
Open
tobiasgrosser wants to merge 3 commits into
Open
feat: prove correctness of the LLVM-dialect RISC-V combines#1043tobiasgrosser wants to merge 3 commits into
tobiasgrosser wants to merge 3 commits into
Conversation
`RISCVCombines/Proofs.lean` covers the combines that rewrite already-selected `riscv` ops, as exact equalities over the total `Data.RISCV.Reg` type. The 63 combines that rewrite `llvm` ops had no data-level lemma at all. Add `RISCVCombines/LLVMProofs.lean` with one `veir_bv_decide` lemma per LLVM combine, named after the rule it justifies. LLVM ops carry poison, so the obligation is a refinement `source ⊒ target` rather than an equality. `bv_decide` bitblasts and so needs concrete widths: these are the `i64` instantiation, using the `i32`/`i64` pair for `sext`/`zext`/`trunc` and `i1` for `select` conditions and `icmp` results. Writing the lemmas surfaced that 18 of these combines were unsound. Each copied a poison-producing flag off a matched op onto a *different* op it creates: `nsw`/`nuw`, `exact`, a `zext`'s `nneg`, or an `or`'s `disjoint`. Dropping a flag only removes poison and is always sound, but transplanting one onto an op with a different poison condition is not. For example, `AndShlShl` kept `nsw` on the created `shl`; with `X = 2^62`, `Y = -2^62`, `Z = 1` the source evaluates to `2^63` while the rewritten program is poison. Fix all 18 in `Combine.lean` by clearing the offending flag on the created op, dropping the minimum: `AndTruncTrunc` and `AndShlShl` still keep `nuw`, since the bits `X & Y` discards are a subset of `Y`'s. With the created flag cleared, the flags read off matched ops decouple from it, so every lemma is stated with those fully free and only the created op carries a literal `false`. Each fix names its reason and points at the lemma; each lemma records the counterexample. Combine coverage goes from 63 to 126 of the 127 registered rules. The one remaining, `li_zero_to_x0`, rewrites `li 0` to a read of `x0`; its obligation is a register-file fact, not a bitvector identity. The existing 82 `RISCVCombines` tests use flag-free inputs and their pass output is byte-identical before and after, so the fix only affects flag-carrying IR. Add `flag_drop_poison_safety.mlir`, which covers nine of the fixed rules and fails against the pre-fix pass.
Contributor
There was a problem hiding this comment.
VeIR Benchmarks
Details
| Benchmark suite | Current: 6a17214 | Previous: c6cca51 | Ratio |
|---|---|---|---|
add-fold-worklist/create |
2274000 ns (± 96777) |
2241000 ns (± 112014) |
1.01 |
add-fold-worklist/rewrite |
3930000 ns (± 35865) |
4033500 ns (± 31734) |
0.97 |
add-fold-worklist-local/create |
2345000 ns (± 93560) |
2246500 ns (± 112038) |
1.04 |
add-fold-worklist-local/rewrite |
3281000 ns (± 44696) |
3391500 ns (± 73095) |
0.97 |
add-zero-worklist/create |
2362000 ns (± 84494) |
2254000 ns (± 55193) |
1.05 |
add-zero-worklist/rewrite |
2628500 ns (± 119883) |
2631000 ns (± 55796) |
1.00 |
add-zero-reuse-worklist/create |
1841000 ns (± 63302) |
1892000 ns (± 67762) |
0.97 |
add-zero-reuse-worklist/rewrite |
2215000 ns (± 79925) |
2198000 ns (± 83020) |
1.01 |
mul-two-worklist/create |
2255000 ns (± 107179) |
2332000 ns (± 98767) |
0.97 |
mul-two-worklist/rewrite |
5631500 ns (± 117948) |
5765000 ns (± 217745) |
0.98 |
add-fold-forwards/create |
2268000 ns (± 82373) |
2219000 ns (± 41445) |
1.02 |
add-fold-forwards/rewrite |
3040000 ns (± 22961) |
3071000 ns (± 21107) |
0.99 |
add-zero-forwards/create |
2302000 ns (± 111431) |
2272000 ns (± 90941) |
1.01 |
add-zero-forwards/rewrite |
1915500 ns (± 42199) |
1989000 ns (± 21300) |
0.96 |
add-zero-reuse-forwards/create |
1863000 ns (± 72929) |
1906000 ns (± 92514) |
0.98 |
add-zero-reuse-forwards/rewrite |
1506000 ns (± 16300) |
1549000 ns (± 15271) |
0.97 |
mul-two-forwards/create |
2230000 ns (± 84884) |
2264000 ns (± 88706) |
0.98 |
mul-two-forwards/rewrite |
3600000 ns (± 55948) |
3769000 ns (± 95668) |
0.96 |
add-zero-reuse-first/create |
1844000 ns (± 112054) |
1890000 ns (± 92368) |
0.98 |
add-zero-reuse-first/rewrite |
8000 ns (± 1749) |
9000 ns (± 2385) |
0.89 |
add-zero-lots-of-reuse-first/create |
1825500 ns (± 60197) |
1916000 ns (± 32593) |
0.95 |
add-zero-lots-of-reuse-first/rewrite |
775000 ns (± 55349) |
836000 ns (± 27463) |
0.93 |
This comment was automatically generated by workflow using github-action-benchmark.
regehr
approved these changes
Jul 9, 2026
`LLVMProofs.lean` proves each LLVM combine's data-level obligation, but nothing tied a theorem to the rewrite of the same name: the lemmas were free-floating `Data.LLVM.Int` refinements, matched to rules by naming convention only. Close that gap for seven combines. Port them from the `PatternRewriter` shape to `LocalRewritePattern` (`RewritePattern.fromLocalRewrite` then performs the insert/replace/erase, which removes their IR-bookkeeping `sorry`s), and prove `LocalRewritePattern.PreservesSemantics` for each in the new `CombineSemantics.lean`. That statement is about the pattern function itself, so the link is now checked by the compiler rather than by convention. Six create no operations: their local pattern returns `(ctx, some (#[], #[v]))`, so `interpretOpList [] state'` is just `state'` and the proof reduces to transporting the matched operands' refinement through the data lemma. `mulo_by_2_unsigned_signed` creates an `llvm.add` and is the op-creating exemplar, additionally replaying that operation forward in the target state. select_same_val_self, select_constant_cmp_true, select_constant_cmp_false, sub_add_reg_x_add_y_sub_y, sub_add_reg_x_add_y_sub_x, trunc_of_zext, mulo_by_2_unsigned_signed New reusable infrastructure, placed per `RewriteProofs/ProofStrategy.md`'s file map: - `matchAdd_getVar?_of_EquationLemmaAt` and `Pure.llvm_add` (Layer 3, `CommonGraphLemmas.lean`), the `add` analogue of `matchNot_getVar?_of_EquationLemmaAt`, which the `sub_add_reg` combines need to recover the defining `add`'s runtime value. - `interpretOp_llvm_binaryInt_forward` (`CommonForwardInterpret.lean`), the LLVM analogue of `interpretOp_riscv_binaryReg_forward`. Every existing forward lemma is RISC-V only; a combine, unlike a lowering, emits `llvm` operations. - `matchTruncOp_interpretOp_unfold` (`CombineSemantics.lean`), the narrowing counterpart of `matchExtOp_interpretOp_unfold`. There is no `Verified.llvm_trunc` bundle, but the pattern's own type guards supply every type fact the proof needs. `bv_decide` bitblasts and so cannot discharge a width-generic goal. The seven patterns therefore carry an `.integerType` guard and a bitwidth guard (`i32`/`i64`; `i32 -> i64 -> i32` for `trunc_of_zext`) so the data lemmas apply, and six of those lemmas are generalized from `Int 64` to `(hw : w = 64 ∨ w = 32)`. The guards narrow the rewrites: they no longer fire at `i8`. `proven_combine_width_guards.mlir` pins both directions. Move `Pure.llvm_zext` and `zext_getVar?_of_EquationLemmaAt` from `LowerSlliuw.lean` to `LowerExt.lean`, next to `matchExtOp_interpretOp_unfold`, their only real dependency. `LowerSlliuw` cannot be imported alongside `LowerBexti` (duplicate generated declaration), which is why `RewriteProofs.lean` omits it; `LowerExt` is already in the build graph. The underlying conflict between those two files is untouched. All seven proofs are `sorry`-free with the axiom footprint `ProofStrategy.md` documents. As in every instruction-selection proof, the four `Return*` predicates are hypotheses, and `fromLocalRewrite` itself still contains `sorry`s, so the chain from `PreservesSemantics` to whole-pass correctness is not yet closed. Pass output is byte-identical on all 82 pre-existing combine tests.
…bines
`(icmp pred X Y) ? X : Y -> {u,s}{max,min} X Y`, eight instances of one shape.
Following `ProofStrategy.md`'s "one proof per lowering shape, not per lowering",
factor them through a single `selectToIMinMaxLocal` combinator parameterized by the
matched predicate and the emitted intrinsic, prove its `PreservesSemantics` once, and
instantiate it eight times. Each instantiation is a single term supplying the
interpreter computation fact, the monotonicity lemma and the value-refinement lemma.
The pattern is DAG-matching: the `icmp` is reached through the `select`'s condition
operand, so recovering its runtime value needs a Layer-3 graph lemma. Add
`matchIcmp_getVar?_of_EquationLemmaAt` (and `Pure.llvm_icmp` in `CommonGraphLemmas.lean`),
the `icmp` analogue of `matchAdd_getVar?_of_EquationLemmaAt`. The emitted intrinsic is
replayed with the existing `interpretOp_llvm_binaryInt_forward`, which needed no change.
Transporting operand refinement through the emitted intrinsic needs `umax`/`umin`/`smax`/
`smin` monotonicity, which the `Data.LLVM.Int` `_mono` family was missing. Add the four,
next to `select_mono`; each is one `grind`, as the rest of the family is.
Two gotchas from `ProofStrategy.md` bit here and are worth recording: `hSemDst` is not
`rfl` for a binary op (the interpreter checks the two operand bitwidths against each
other), and with the createOp bind plus three equality guards still in `hpattern`, the
structural `grind`s blow up -- the `op.getResults` equation is now established before any
peeling, while `hpattern` is still small.
As with the previous seven, the patterns carry `.integerType` and `i32`/`i64` bitwidth
guards so the `veir_bv_decide` data lemmas apply, and the eight data lemmas are
generalized from `Int 64` to `(hw : w = 64 ∨ w = 32)`. The rewrites no longer fire at
`i8`; `proven_combine_width_guards.mlir` pins that.
Graph-level coverage: 7 -> 15 of the 127 registered combines. All fifteen are sorry-free
with the axiom footprint `ProofStrategy.md` documents. `Combine.lean` drops from 254 to
230 sorry-lines. The registered pattern list is unchanged and pass output is
byte-identical on all 82 pre-existing combine tests.
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.
RISCVCombines/Proofs.leancovers the combines that rewrite already-selectedriscvops, as exact equalities over the totalData.RISCV.Regtype. The 63 combines that rewritellvmops had no data-level lemma at all — half ofCombine.leanwas unproven.Lemmas
New
RISCVCombines/LLVMProofs.lean: oneveir_bv_decidelemma per LLVM combine, named after the rule it justifies (so the naming convention that linksProofs.leantoCombine.leanextends to it).LLVM ops carry poison, so the obligation is a refinement rather than an equality — the rewritten value may be more defined than the original, never less:
bv_decidebitblasts and so cannot handle generic widths. These are thei64instantiation:sext/zextfromi32toi64,truncfromi64toi32, andi1forselectconditions andicmpresults.18 unsound combines, fixed
Writing the lemmas surfaced that 18 combines were unsound. Each copied a poison-producing flag off a matched op onto a different op it creates. Dropping a flag only removes poison and is always sound; transplanting one onto an op with a different poison condition is not.
nswon a hoistedshl/trunc—AndShlShl,AndTruncTrunc, and theOr/Xorvariants.nuw/exacton hoistedOr/Xor—Xalone can supply the discarded bit that poisons the created op while the source is fine. (Andis safe: its bits are a subset ofY's.)disjointonor— a second, independent failure inOrTruncTrunc,OrShlShl,OrLshrLshr,OrAshrAshr,OrAndAnd:XandYmay overlap only in bits the hoisted op discards.nnegon a hoistedzext—OrZextZext,XorZextZext.sub_add_reg_x_sub_y_add_x,sub_add_reg_x_sub_x_add_y,add_shift,add_shift_commute,sub_one_from_sub_rw.Concrete counterexamples, verified by
#eval(each prints(source, target)as(value, poison)):AndShlShlnswX = 2^62,Y = -2^62,Z = 1→ source2^63, targetpoisonAndTruncTruncnswX = 2^31,Y = -1→ source0x80000000, targetpoisonOrAndAnddisjointX = 1,Y = -1,Z = -2→ source-2, targetpoisonsub_one_from_sub_rwnuwA = 5,B = 3→ source1, targetpoisonCombine.leannow clears the offending flag on the created op, dropping the minimum.AndTruncTruncandAndShlShlstill keepnuw, since the bitsX & Ydiscards are a subset ofY's. Each fix carries a comment naming its reason and pointing at its lemma; each lemma records the counterexample that motivated the clear.With the created flag cleared, the flags read off matched ops decouple from it, so each lemma is stated with those fully free and only the created op carries a literal
false:Coverage
63 → 126 of the 127 registered rules. The one remaining,
li_zero_to_x0, rewritesli 0to a read ofx0; its obligation is a register-file fact, not a bitvector identity.These are still standalone data-level identities — nothing yet ties
AndSextSext-the-theorem toAndSextSext-the-rule, andCombine.leanstill discharges its IR-bookkeeping obligations withsorry. Same gap as the existingProofs.lean; wiring the two together is follow-up work.Testing
sorry-free and depend only onpropext,Classical.choice,Quot.sound— nosorryAx, nonative_decide.RISCVCombinestests use flag-free inputs;veir-opt -p=riscv-combineoutput is byte-identical across all 82 before and after, so the fix only affects flag-carrying IR.flag_drop_poison_safety.mlircovers nine of the fixed rules and fails against the pre-fix pass (checked), including thatAndShlShlturnsoverflowFlags = 3intooverflowFlags = 2rather than droppingnuwtoo.lake test, and a warning-cleanlake build Veir(touched modules also built individually).🤖 Generated with Claude Code