[PATCH] D125845: [InstCombine] Canonicalize GEP of GEP by swapping constant-indexed GEP to the back

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/gep-canonicalize-constant-indices.ll:33
 ;
   %1 = getelementptr inbounds [4 x i32], ptr %p, i64 %a, i64 1
   %2 = getelementptr inbounds i32, ptr %p, i64 %b
----------------
spatel wrote:
> spatel wrote:
> > I don't know what this test and the next one are supposed to show. %1 is dead code, so it gets eliminated before anything else might have happened.
> I'm more confused now. The test had 1 GEP, but now it has 2 - how is that better?
> I'm more confused now. The test had 1 GEP, but now it has 2 - how is that better?

This question is still not answered afaict. We now get two non-inbound GEP:s instead of one inbounds GEP. Given that there are no other GEP:s etc that can be simplified (as in this test case) that doesn't look like an improvement?

And I guess loosing the inbounds property can be bad in general when doing this canonicalizations. Do you have any examples when you see a performance improvement when rewriting inbound GEP:s into non-inbound GEP:s? Given the amount of complaints about seen regressions it would be nice to hear what kinds of benchmarks that were done and if you tried to limit it to cases when instruction count didn't increase, or when it would turn X inbound GEP:s into X or more non-inbound GEP:s.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125845