[PATCH] D144445: (WIP) [AggressiveInstCombine] folding load for constant global patterened arrays and structs

[Kohei Asano via Phabricator via llvm-commits]

khei4 planned changes to this revision.
khei4 added a comment.

Thank you for the review! I may fuse the GEP-related plans I proposed! because I might have misunderstood gep!



================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:845
+  // FIXME: This and following loop inner checks seems too pattern specific. Any
+  // better way to judge the pointer arrives back to Global Variable?
+  if (auto *Call = dyn_cast<CallBase>(PtrOp))
----------------
nikic wrote:
> What you want to do is after the GEPOperator loop, check that the final PtrOpV is GV. This makes sure there were only GEPs.
Thanks! I'll check that before and after the loop!


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:864
+  if (auto *GEP = dyn_cast<GEPOperator>(PtrOp)) {
+    Stride = APInt(BW, DL.getTypeAllocSize(GEP->getSourceElementType()));
+    Value *PtrOpV = GEP;
----------------
nikic wrote:
> This shouldn't be needed when using `std::optional`.
I might have missed some patterns, but I guess it's necessary because GCD (self-referencing) calculations need its initial value as the first type size. :)


```
      Stride = APIntOps::GreatestCommonDivisor(
          Stride,
          APInt(ConstOffset.getBitWidth(), IndexTypeSize.getZExtValue()));
```


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:871
+    if (!GEP->isInBounds())
+      return false;
+
----------------
nikic wrote:
> You should probably extract the whole "get stride of GEP" logic into a separate function. That way, all these early bailouts don't apply for the alignment case (where we don't care about inbounds).
(with above comment)
Ah, OK I might misunderstand the behavior of the load with non-inbounds GEP indices, but correct alignment! 


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

https://reviews.llvm.org/D144445