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

Mon Nov 28 11:04:13 PST 2022

davidxl 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
----------------
bjope wrote:
> 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.
The changes in this test seems to be irrelevant. The original test was probably buggy (with dead code), and the change here just fixed the test. (not sure if the main patch will affect the output though).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125845