[PATCH] D146622: [AggressiveInstCombine] folding load for constant global patterened arrays and structs by GEP indices
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 8 07:37:19 PDT 2023
nikic added a comment.
Logic looks good, some style nits. Can you please check the compile-time impact for this patch?
================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:824-825
+// calculate GEP Stride and accumulated const ModOffset. return Stride and
+// ModOffset
+static std::pair<APInt, APInt>
----------------
================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:828
+getStrideAndModOffsetOfGEP(Value *PtrOp, const DataLayout &DL) {
+
+ unsigned BW = DL.getIndexTypeSizeInBits(PtrOp->getType());
----------------
nit: Unnecessary empty line.
================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:833
+ // Return a minimum gep stride, greatest common divisor of consective gep
+ // indices type sizes (c.f. Bézout's identity).
+ while (auto GEP = dyn_cast<GEPOperator>(PtrOp)) {
----------------
================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:834
+ // indices type sizes (c.f. Bézout's identity).
+ while (auto GEP = dyn_cast<GEPOperator>(PtrOp)) {
+ MapVector<Value *, APInt> VarOffsets;
----------------
================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:847-848
+ else
+ Stride = APIntOps::GreatestCommonDivisor(
+ *Stride, APInt(BW, Scale.getZExtValue()));
+ }
----------------
I don't think there's a need to create a separate APInt here?
================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:855
+ // Check whether pointer arrives back at Global Variable.
+ // Even if it's not, we can check by alignment.
+ if (!isa<GlobalVariable>(PtrOp))
----------------
================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:884
Constant *C = GV->getInitializer();
+ unsigned BW = DL.getIndexTypeSizeInBits(PtrOp->getType());
----------------
I think you can move BW into the if below now, it's not used otherwise.
================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:898
+ LA <= GV->getAlign().valueOrOne() &&
+ (!Stride || Stride.getZExtValue() < LA.value())) {
+ ConstOffset = APInt(BW, 0);
----------------
I think the `!Stride` isn't needed anymore.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146622/new/
https://reviews.llvm.org/D146622
More information about the llvm-commits
mailing list