[PATCH] D125438: [InstCombine] NEW Baseline tests for InstCombine optimization to merge GEP instructions with constant indices

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/gep-canonicalize-constant-indices.ll:62
+
+; It is valid to swap if the source operand of the first GEP has multiple uses.
+define ptr @multipleUses1(ptr %p) {
----------------
Isn't this test the same as multipleUses3? The comment on multipleUses3 seems correct (not valid to swap).


================
Comment at: llvm/test/Transforms/InstCombine/gep-canonicalize-constant-indices.ll:88
+  %3 = insertvalue {ptr, ptr} undef, ptr %2, 0
+  %4 = insertvalue {ptr, ptr} %3, ptr %2, 1
+  ret {ptr, ptr} %4
----------------
The usual way to add extra uses is `call void @use(ptr %2)`.


================
Comment at: llvm/test/Transforms/InstCombine/gep-merge-constant-indices.ll:162
+
+; result = (i16*) &((struct.B*) (p + a))[0].member1 + 4
+define ptr @appendIndexReverse(ptr %p) {
----------------
What's `+ a` in this comment?


================
Comment at: llvm/test/Transforms/InstCombine/gep-merge-constant-indices.ll:172
+  ret ptr %2
+}
----------------
Maybe I missed it, but it would be good to test a case where neither element type can be used, e.g. `gep i24, 1` and `gep i32, 1`. Total offset is 7, which is not a multiple of 3 or 4.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125438/new/

https://reviews.llvm.org/D125438