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

[Kohei Asano via Phabricator via llvm-commits]

khei4 added a comment.

Thank you for the review!



================
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,
----------------
nikic wrote:
> 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.
Thanks for the good catch!


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:854
+
+    for (auto [V, IndexTypeSize] : VarOffsets) {
+      APInt Scale = APInt(BW, IndexTypeSize.getZExtValue());
----------------
nikic wrote:
> This looks confused. The second element in the pair is not the IndexTypeSize, but already the Scale (as an APInt).
Sounds reasonable! (I thought this bit width casting is necessary to avoid mismatching on APInt ops, but it's not.)


================
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);
----------------
nikic wrote:
> I think we can keep this non-optional. `Stride=1` is always valid as a fallback.
yeah, but the GCD calculation process seems good to be undefined on the first.
I'll assign 1 on the following Stride's "nullopt" checking!


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

https://reviews.llvm.org/D146622