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

[Nikita Popov via Phabricator via llvm-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

Nearly there, I think...



================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:831
+static bool foldPatternedLoads(Instruction &I, const DataLayout &DL) {
+
+  auto *LI = dyn_cast<LoadInst>(&I);
----------------
Unnecessary/unusual newline.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:838
+  // initializer. Skip expensive logic if this is not the case.
+  auto *PtrOp = LI->getOperand(0);
+  auto *GV = dyn_cast<GlobalVariable>(getUnderlyingObject(PtrOp));
----------------
getPointerOperand()` is a bit more elegant.


================
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))
----------------
What you want to do is after the GEPOperator loop, check that the final PtrOpV is GV. This makes sure there were only GEPs.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:854
+  // Bail for large initializers in excess of 4K to avoid allocating
+  // too much memory.
+  uint64_t GVSize = DL.getTypeAllocSize(C->getType());
----------------
"to avoid scanning too much memory" maybe -- we're not really allocating anything here.


================
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;
----------------
This shouldn't be needed when using `std::optional`.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:869
+    // equivalent by just seeing a global variable. We need to check for
+    // consective GEP.
+    if (!GEP->isInBounds())
----------------
I don't really get what this comment is trying to say. The reason why non-inbounds GEP is tricky to handle is that there is an implicit modulo the address space size, which makes it harder to calculate the GCD.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:871
+    if (!GEP->isInBounds())
+      return false;
+
----------------
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).


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

https://reviews.llvm.org/D144445