Skip to content

Feat: cse for gep incl. incomplete testing#887

Draft
salinhkuhn wants to merge 2 commits into
mainfrom
feat-gep-cse
Draft

Feat: cse for gep incl. incomplete testing#887
salinhkuhn wants to merge 2 commits into
mainfrom
feat-gep-cse

Conversation

@salinhkuhn

Copy link
Copy Markdown

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:

  • need to add the file check

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@regehr

regehr commented Jun 17, 2026

Copy link
Copy Markdown
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 gep inbounds with a regular gep

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

@tobiasgrosser

Copy link
Copy Markdown
Collaborator

@salinhkuhn, is this now ready for review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants