[PATCH] D125845: [InstCombine] Canonicalize GEP of GEP and merging GEP with constant indices

[William Junda Huang via Phabricator via llvm-commits]

huangjd marked 4 inline comments as done.
huangjd added inline comments.


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


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2054
+         !isa<ConstantInt>(*(GEP.indices().end() - 1))) ||
+        (Src->hasAllConstantIndices() && !GEP.hasAllConstantIndices())) {
+      bool InBounds = isMergedGEPInBounds(*Src, *cast<GEPOperator>(&GEP));
----------------
nikic wrote:
> I would recommend to initially only keep the `Src->hasAllConstantIndices() && !GEP.hasAllConstantIndices()` case. The profitability of this transform for the case where both GEPs are non-constant is not really clear.
If Src is not all constant index and GEP is all constant index, merging them can reduce one GEP. I have observed that in the backend, inst selection does not always generate the most simplified output for multiple GEP instructions, so it's better to reduce the IR count


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125845