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

[William Junda Huang via Phabricator via llvm-commits]

huangjd added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2040
+        (Src->hasAllConstantIndices() && !GEP.hasAllConstantIndices())) {
+      bool InBounds = isMergedGEPInBounds(*Src, *cast<GEPOperator>(&GEP));
+      Value *NewSrc = Builder.CreateGEP(
----------------
nikic wrote:
> Unfortunately, this is not sufficient to preserve inbounds. If the indices have different sign, then swapping them may make the GEP non-inbounds even if both original GEPs were inbounds.
> 
> `gep inbounds (gep inbounds p, -1), X -> `gep inbounds (gep inbounds p, X), -1` is not generally valid.
I have seen the same inbounds check used in another place (line 2071), so that may be problematic as well? I think we could add another check in isMergedGEPInBounds to require both GEP offsets are known to be same sign? 


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1981
+          shouldCanonicalizeSwap = false;
+        }
+
----------------
nikic wrote:
> huangjd wrote:
> > nikic wrote:
> > > This is an interesting case. An alternative would be to skip this code if GEP has all constant indices, i.e. to say that `gep (gep p, x), C` is always the canonical form, even if it were possible to move `(gep p, C)` out of the loop. This makes some sense in that `gep p, C` is typically free (folded into addressing mode). I'm not sure whether that would really be better though.
> > LICM is always beneficial as it guarantees the reduction of runtime instruction count, but canonicalization can't guarantee an optimization
> The main thing I'd be concerned about here is that the loop invariance canonicalization is only performed if cached LoopInfo is available. This means that depending on whether a particular InstCombine run has LoopInfo available, we will switch back and forth between two different orders.
Since these two transforms contradict each other, one of them has to take precedence when both are available. If loopInfo is available during any pass and it swaps out the inner GEP, then further Instcombine pass shouldn't be able to swap it back. Perhaps I should add a check that canonicalization only takes place if both GEP are on the same BB


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125845