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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:831-844
+  if (isa<GlobalVariable>(PtrOp)) {
+    ModOffset = APInt(BW, 0);
+    Stride = APInt(BW, 1);
+    return true;
+  }
+
+  // If PtrOp is neither GlobalVariable nor GEP, we cannot use GEP stride,
----------------
I think these two ifs are redundant with the `!isa<GlobalVariable>(PtrOpV)` check that's done below the while loop. It should be possible to just drop them.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:854
+
+    for (auto [V, IndexTypeSize] : VarOffsets) {
+      APInt Scale = APInt(BW, IndexTypeSize.getZExtValue());
----------------
This looks confused. The second element in the pair is not the IndexTypeSize, but already the Scale (as an APInt).


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:878
+  // In consideration of signed GEP indices, non-negligible offset become
+  // remainder of divission by minimum GEP stride.
+  ModOffset = ModOffset.srem(*Stride);
----------------
divission -> division


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:909
   unsigned BW = DL.getIndexTypeSizeInBits(PtrOp->getType());
-  // TODO: Determine stride based on GEPs.
-  APInt Stride(BW, 1);
+  std::optional<APInt> Stride;
   APInt ConstOffset(BW, 0);
----------------
I think we can keep this non-optional. `Stride=1` is always valid as a fallback.


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

https://reviews.llvm.org/D146622