Feat: cse for gep incl. incomplete testing#887
Draft
salinhkuhn wants to merge 2 commits into
Draft
Conversation
Contributor
There was a problem hiding this comment.
VeIR Benchmarks
Details
| Benchmark suite | Current: 5de1119 | Previous: 936c95c | Ratio |
|---|---|---|---|
add-fold-worklist/create |
1083000 ns (± 22808) |
2348000 ns (± 97457) |
0.46 |
add-fold-worklist/rewrite |
2023000 ns (± 34825) |
4141000 ns (± 38823) |
0.49 |
add-fold-worklist-local/create |
1108000 ns (± 16906) |
2291000 ns (± 44019) |
0.48 |
add-fold-worklist-local/rewrite |
1783000 ns (± 26013) |
3404000 ns (± 88004) |
0.52 |
add-zero-worklist/create |
1128000 ns (± 56839) |
2372500 ns (± 138531) |
0.48 |
add-zero-worklist/rewrite |
1319000 ns (± 30762) |
2552000 ns (± 62280) |
0.52 |
add-zero-reuse-worklist/create |
936000 ns (± 21260) |
2026500 ns (± 127621) |
0.46 |
add-zero-reuse-worklist/rewrite |
1073000 ns (± 19204) |
2122000 ns (± 69455) |
0.51 |
mul-two-worklist/create |
1109000 ns (± 23900) |
2292000 ns (± 111491) |
0.48 |
mul-two-worklist/rewrite |
2844000 ns (± 62219) |
5580000 ns (± 86102) |
0.51 |
add-fold-forwards/create |
1086000 ns (± 91972) |
2422000 ns (± 118219) |
0.45 |
add-fold-forwards/rewrite |
1577500 ns (± 514693) |
2986000 ns (± 45152) |
0.53 |
add-zero-forwards/create |
1084000 ns (± 18152) |
2488000 ns (± 113288) |
0.44 |
add-zero-forwards/rewrite |
982000 ns (± 31230) |
1997000 ns (± 35374) |
0.49 |
add-zero-reuse-forwards/create |
931000 ns (± 3564) |
1865000 ns (± 10756) |
0.50 |
add-zero-reuse-forwards/rewrite |
798000 ns (± 9762) |
1537000 ns (± 16432) |
0.52 |
mul-two-forwards/create |
1088000 ns (± 16239) |
2287000 ns (± 42759) |
0.48 |
mul-two-forwards/rewrite |
1837000 ns (± 37785) |
3584000 ns (± 30971) |
0.51 |
add-zero-reuse-first/create |
904000 ns (± 24224) |
1983000 ns (± 107910) |
0.46 |
add-zero-reuse-first/rewrite |
9000 ns (± 447) |
8000 ns (± 1840) |
1.13 |
add-zero-lots-of-reuse-first/create |
909000 ns (± 17978) |
1920500 ns (± 103233) |
0.47 |
add-zero-lots-of-reuse-first/rewrite |
415000 ns (± 5983) |
776000 ns (± 30418) |
0.53 |
This comment was automatically generated by workflow using github-action-benchmark.
Collaborator
|
thanks Sarah! I believe this change is correct, but please add a test case showing that it works and maybe also one showing that it does not merge a also note that this version of CSE is pretty weak, it does not work across basic blocks-- there's a problem with the dominator analaysis that needs to be fixed before this is a somewhat useful CSE pass |
Collaborator
|
@salinhkuhn, is this now ready for review? |
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.
This PR adds GEP to the CSE pass. In the output of #886 I noticed that a series of identical GEPs appear namely the same getelementptr (same base, index, elem_type, and noWrapFlags) recomputed many times within a single basic block. The change routes adds getelementptr to the existing Veir/Passes/CSE.lean. GEP is pure pointer arithmetic (afaiu, https://llvm.org/docs/GetElementPtr.html).
Testing still incomplete: