[PATCH] D125845: [InstCombine] Canonicalize GEP of GEP and merging GEP with constant indices
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 18 05:56:34 PDT 2022
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
More information about the llvm-commits
mailing list