test(cse): cover getelementptr incl. elemtype / noWrapFlags negative cases

[salinhkuhn]

This PR adds missing test coverage for GEP in the CSE pass. GEP is pure pointer arithmetic (https://llvm.org/docs/GetElementPtr.html), so identical getelementptr ops (same base, index, elem_type, and noWrapFlags) within a block can be merged, which CSE already does since #867, but there was no test for it yet.

This PR adds tests that check:

• two identical inbounds GEPs get merged into one
• a GEP with a different elem_type is not merged
• a plain GEP (noWrapFlags = 0) is not merged with the inbounds one (noWrapFlags = 3), since they differ in UB semantics

[github-actions]

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]

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]

@salinhkuhn, is this now ready for review?

[tobiasgrosser]

This has not been worked on since a while and is literally a one-line change. Let's close this for now. @salinhkuhn, please reopen when you have time to work on this.

[salinhkuhn]

I updated the PR to clarify the confusion about not testing the inbounds GEP, tests are added for both plain GEP and inbounds GEP. From my side, @regehr's comments were resolved, so I'm happy for a review. I'm not clear on what exactly the "one-line change" is referring to, but I'm happy to implement it as soon as I understand what it is and to close this properly with no work remaining. @tobiasgrosser

[tobiasgrosser]

@salinhkuhn, this is great. If you feel this PR is ready to review, you should (a) rebase it on latest main to resolve merge conflicts, (b) undraft it, and (c) write a summarizing comment of where you are standing. You did (c) now. Can you do (a) and (b). Then @regehr can give it another review.

[salinhkuhn]

@salinhkuhn, this is great. If you feel this PR is ready to review, you should (a) rebase it on latest main to resolve merge conflicts, (b) undraft it, and (c) write a summarizing comment of where you are standing. You did (c) now. Can you do (a) and (b). Then @regehr can give it another review.

Done. Given the progress on VeIR, my change isn't needed anymore after hew rebase on main (good news too :) ), except for the tests, which aren't on main yet! (I updated the PR description.)

[tobiasgrosser]

@salinhkuhn, can you update your PR message to not mention the full history but just contain information on what this PR does today. E.g., "this PR adds missing test coverage for GEP CSE." (in more detail)

[salinhkuhn]

@salinhkuhn, can you update your PR message to not mention the full history but just contain information on what this PR does today. E.g., "this PR adds missing test coverage for GEP CSE." (in more detail)

yup done now!

[tobiasgrosser]

This LGTM. Let's see what @regehr says.