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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a reviewer: aeubanks.
nikic added inline comments.


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


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2039
+      }))
+    return nullptr;
+
----------------
Doing this is not necessary: InstCombine is not supposed to gracefully deal with invalid IR. I've removed the offending test in https://github.com/llvm/llvm-project/commit/128da94d38242c28e6bf23ad025e0cb2d6ce9e4f.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2048
+       Src->getPointerOperand()->getType() ==
+           GEP.getPointerOperand()->getType())) {
+    // When swapping, GEP with all constant indices are more prioritized than
----------------
You probably don't need the explicit isOpaquePointerTy() check here, that should be covered by the type equality check.


================
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));
----------------
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.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2060
+      if (GetElementPtrInst *NewGEP = dyn_cast<GetElementPtrInst>(NewSrc))
+        NewGEP->setIsInBounds(InBounds);
+      GetElementPtrInst *NewGEP = GetElementPtrInst::Create(
----------------
You can pass InBounds directly to CreateGEP, as the last argument.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2121
+                Builder.getInt8Ty(), Src->getOperand(0),
+                Builder.getInt(OffsetOld), GEP.getName());
       return nullptr;
----------------
This should be a separate patch, it's an independent transform.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125845