[PATCH] D146622: [AggressiveInstCombine] folding load for constant global patterened arrays and structs by GEP indices

[Nikita Popov via Phabricator via llvm-commits]

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