feat(rewriter): RAUW + erase is safe#936
Open
regehr wants to merge 2 commits into
Open
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
VeIR Benchmarks
Details
| Benchmark suite | Current: d80b763 | Previous: e0dd1da | Ratio |
|---|---|---|---|
add-fold-worklist/create |
2183000 ns (± 91792) |
1849000 ns (± 110957) |
1.18 |
add-fold-worklist/rewrite |
3994000 ns (± 56937) |
3562500 ns (± 445089) |
1.12 |
add-fold-worklist-local/create |
2192000 ns (± 89456) |
1795000 ns (± 90623) |
1.22 |
add-fold-worklist-local/rewrite |
3287000 ns (± 133653) |
2927500 ns (± 38233) |
1.12 |
add-zero-worklist/create |
2241000 ns (± 22061) |
1795000 ns (± 84568) |
1.25 |
add-zero-worklist/rewrite |
2536000 ns (± 16682) |
2245000 ns (± 18507) |
1.13 |
add-zero-reuse-worklist/create |
1883000 ns (± 91767) |
1582000 ns (± 30458) |
1.19 |
add-zero-reuse-worklist/rewrite |
2100000 ns (± 50946) |
1853000 ns (± 55162) |
1.13 |
mul-two-worklist/create |
2225000 ns (± 66530) |
1819000 ns (± 194448) |
1.22 |
mul-two-worklist/rewrite |
5628000 ns (± 246524) |
5018000 ns (± 876074) |
1.12 |
add-fold-forwards/create |
2218000 ns (± 58354) |
1829000 ns (± 44579) |
1.21 |
add-fold-forwards/rewrite |
3112000 ns (± 109266) |
2680000 ns (± 63811) |
1.16 |
add-zero-forwards/create |
2200000 ns (± 69702) |
1802000 ns (± 21534) |
1.22 |
add-zero-forwards/rewrite |
2007000 ns (± 31485) |
1707000 ns (± 20724) |
1.18 |
add-zero-reuse-forwards/create |
1854000 ns (± 57483) |
1576000 ns (± 62998) |
1.18 |
add-zero-reuse-forwards/rewrite |
1555000 ns (± 31706) |
1407000 ns (± 46848) |
1.11 |
mul-two-forwards/create |
2229000 ns (± 55577) |
1845000 ns (± 76020) |
1.21 |
mul-two-forwards/rewrite |
3712000 ns (± 51257) |
3235000 ns (± 59450) |
1.15 |
add-zero-reuse-first/create |
1794000 ns (± 56769) |
1582000 ns (± 48201) |
1.13 |
add-zero-reuse-first/rewrite |
8000 ns (± 0) |
10000 ns (± 1697) |
0.80 |
add-zero-lots-of-reuse-first/create |
1859500 ns (± 74214) |
1597500 ns (± 45792) |
1.16 |
add-zero-lots-of-reuse-first/rewrite |
789000 ns (± 40023) |
780000 ns (± 38323) |
1.01 |
This comment was automatically generated by workflow using github-action-benchmark.
math-fehr
reviewed
Jun 29, 2026
math-fehr
left a comment
math-fehr
left a comment
Collaborator
There was a problem hiding this comment.
Thanks! I left some comments, but otherwise I feel that's going to be quite useful.
also, as I mentionned in the comments, I feel we should just fix eraseOp!
Comment on lines
+218
to
+239
| @[simp, grind =] | ||
| theorem replaceValue_ctx {rewriter : PatternRewriter OpInfo} {oldVal newVal : ValuePtr} | ||
| {neValues : oldVal ≠ newVal} {oldIn : oldVal.InBounds rewriter.ctx.raw} | ||
| {newIn : newVal.InBounds rewriter.ctx.raw} : | ||
| (rewriter.replaceValue oldVal newVal neValues oldIn newIn).ctx | ||
| = WfRewriter.replaceValue rewriter.ctx oldVal newVal neValues oldIn newIn := by | ||
| simp [replaceValue, addUsersInWorklist_same_ctx] | ||
|
|
||
| /-- After replacing `op`'s single result with `newValue`, erasing `op` succeeds: the three | ||
| preconditions of `eraseOp` (`op` in bounds, no regions, no uses) hold in the resulting rewriter. -/ | ||
| theorem eraseOp_some_after_replace_result0 | ||
| (rewriter : PatternRewriter OpInfo) (op : OperationPtr) (newValue : ValuePtr) | ||
| (hop : op.InBounds rewriter.ctx.raw) | ||
| (hres : (op.getResult 0 : ValuePtr).InBounds rewriter.ctx.raw) | ||
| (hnew : newValue.InBounds rewriter.ctx.raw) | ||
| (hne : (op.getResult 0 : ValuePtr) ≠ newValue) | ||
| (hone : op.getNumResults! rewriter.ctx.raw = 1) | ||
| (hregions : op.getNumRegions! rewriter.ctx.raw = 0) : | ||
| ∃ rewriter', | ||
| (rewriter.replaceValue (op.getResult 0 : ValuePtr) newValue hne hres hnew).eraseOp op | ||
| = some rewriter' := | ||
| ⟨_, rfl⟩ |
Collaborator
There was a problem hiding this comment.
Can you move these lemmas to a new file PatternRewriter/Lemmas.lean (unless one exist already?)
Comment on lines
+228
to
+239
| theorem eraseOp_some_after_replace_result0 | ||
| (rewriter : PatternRewriter OpInfo) (op : OperationPtr) (newValue : ValuePtr) | ||
| (hop : op.InBounds rewriter.ctx.raw) | ||
| (hres : (op.getResult 0 : ValuePtr).InBounds rewriter.ctx.raw) | ||
| (hnew : newValue.InBounds rewriter.ctx.raw) | ||
| (hne : (op.getResult 0 : ValuePtr) ≠ newValue) | ||
| (hone : op.getNumResults! rewriter.ctx.raw = 1) | ||
| (hregions : op.getNumRegions! rewriter.ctx.raw = 0) : | ||
| ∃ rewriter', | ||
| (rewriter.replaceValue (op.getResult 0 : ValuePtr) newValue hne hres hnew).eraseOp op | ||
| = some rewriter' := | ||
| ⟨_, rfl⟩ |
Collaborator
There was a problem hiding this comment.
Well, I realize from your theorem that my definition of eraseOp returned an option for no reasons ^^'
eraseOp should always succeed, and your proof is essentially not even using your hypotheses to be proven.
I think we should then rather have a PR that removes the Option from eraseOp here, and that should make things easier!
Comment on lines
+888
to
+891
| {op : OperationPtr} {newValue : ValuePtr} | ||
| {hres : (op.getResult 0 : ValuePtr).InBounds ctx.raw} | ||
| {hnew : newValue.InBounds ctx.raw} | ||
| {hne : (op.getResult 0 : ValuePtr) ≠ newValue} |
Collaborator
There was a problem hiding this comment.
I believe you can just remove these parameters, as they can be inferred from the goal. That should make this a bit nicer to read
Comment on lines
+906
to
+918
| theorem OperationPtr.erase_preconditions_after_replace_result0 | ||
| {op : OperationPtr} {newValue : ValuePtr} | ||
| (hop : op.InBounds ctx.raw) | ||
| (hres : (op.getResult 0 : ValuePtr).InBounds ctx.raw) | ||
| (hnew : newValue.InBounds ctx.raw) | ||
| (hne : (op.getResult 0 : ValuePtr) ≠ newValue) | ||
| (hone : op.getNumResults! ctx.raw = 1) | ||
| (hregions : op.getNumRegions! ctx.raw = 0) : | ||
| let ctx' := | ||
| WfRewriter.replaceValue ctx (op.getResult 0 : ValuePtr) newValue hne hres hnew | ||
| op.InBounds ctx'.raw ∧ | ||
| op.getNumRegions! ctx'.raw = 0 ∧ | ||
| op.hasUses! ctx'.raw = false := by |
Collaborator
There was a problem hiding this comment.
I'm not sure we overall need this theorem?
Mostly because each component is already a simp/grind lemma?
| have hidx0 : index = 0 := by omega | ||
| subst hidx0 | ||
| exact ValuePtr.hasUses!_WfRewriter_replaceValue_oldValue | ||
|
|
Collaborator
There was a problem hiding this comment.
Can you add these to a new Rewriter/WfRewriter/Lemmas.lean?
I'm trying to have GetSet.lean being very mechanical, and this is slightly outside of its scope!
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
2 participants
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.
Mathieu if you have time to give me feedback on this I would appreciate it.
I was going to do more work on Verifier.lean but it seems like maybe starting here is more useful, and then I can go back to the verifier lemmas once I know more about what exactly they must provide