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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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(
----------------
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.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1981
+          shouldCanonicalizeSwap = false;
+        }
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125845