[PATCH] D127392: [AggressiveInstCombine] Combine consecutive loads which are being merged to form a wider load.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:488
+  Value *Shift = nullptr;
+  Type *Zext;
+  AAMDNodes AATags;
----------------
Zext -> ZextType, maybe?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:495
+// (ZExt(L1) << shift1) | ZExt(L2) -> ZExt(L3)
+static bool foldLoadsIterative(Value *V, LoadOps &LOps, const DataLayout &DL,
+                               AliasAnalysis *AA) {
----------------
Iterative -> Recursive


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:567
+    if (++NumScanned > MaxInstrsToScan)
+      return false;
+  }
----------------
This check should come first, otherwise you don't count store instructions.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:591
+  uint64_t PrevSize =
+      DL.getTypeStoreSize(IntegerType::get(LI1->getContext(), LoadSize1));
+  if ((Shift2 - Shift1) != ShiftDiff || (Offset2 - Offset1) != PrevSize)
----------------
I think this handles non-byte-sized loads incorrectly. Let's say you do i4 loads with 1 byte offset. Then PrevSize will be 1 and match the offset, even though the loads are not actually consecutive.

Please add some tests for non-byte-sized loads.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:624
+  IRBuilder<> Builder(&I);
+  LoadInst *NewLoad, *LI1;
+  LI1 = LOps.Root;
----------------
Combine these declarations with initialization.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:645
+                   Load1Ptr, "", LI1->isVolatile(), LI1->getAlign(),
+                   LI1->getOrdering(), LI1->getSyncScopeID());
+
----------------
Why does this not use the IRBuilder?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:692
+      MadeChange |=
+          foldConsecutiveLoads(I, F.getParent()->getDataLayout(), TTI, AA);
     }
----------------
The DataLayout fetch can be moved outside the loop.


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

https://reviews.llvm.org/D127392