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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

I feel like this being recursive makes it more difficult to reason about than it could be. I have added some mostly nitpicks/cleanups below.



================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:438
+  Value *Shift = nullptr;
+  Type *zext;
+};
----------------
zext -> Zext (Or ZExt? Capitalized at least)


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:443
+// 1. (zExt(L1) << shift1) | (zExt(L2) << shift2) -> zExt(L3) << shift1
+// 2. (? | (zExt(L1) << shift1)) | (zExt(L2) << shift2)
+//  -> ? | (zExt(L3) << shift1)
----------------
Are there any tests for case 2?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:462-463
+
+  LoadInst *LI1, *LI2;
+  Value *ShAmt1 = nullptr;
+
----------------
The variables can be defined where they are first used.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:477
+  // Check if loads are same, atomic or volatile.
+  if (LI1 == LI2 || !LI1 || !LI2 || (!LI1->isSimple() && !LI2->isSimple()))
+    return false;
----------------
Should it check that the address space is the same?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:497
+  Value *Load1Ptr = LI1->getOperand(0);
+  if (isa<BitCastInst>(Load1Ptr) || isa<GetElementPtrInst>(Load1Ptr)) {
+    APInt Offset(DL.getIndexTypeSizeInBits(Load1Ptr->getType()), 0);
----------------
Does it matter if it is a bitcast or a gep?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:515
+  // Verify if both loads have same base pointers and load sizes are same.
+  uint64_t loadSize1 = DL.getTypeStoreSizeInBits(LI1->getType());
+  uint64_t loadSize2 = DL.getTypeStoreSizeInBits(LI2->getType());
----------------
Capitalize variable names.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:544
+  // are consecutive.
+  uint64_t ShiftDiff = IsBigEndian ? loadSize2 : loadSize1;
+  uint64_t PrevSize =
----------------
We checked that loadSize1 == loadSize2 above.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:547
+      DL.getTypeStoreSize(IntegerType::get(LI1->getContext(), loadSize1));
+  if (!(((Shift2 - Shift1) == ShiftDiff) && ((Offset2 - Offset1) == PrevSize)))
+    return false;
----------------
Demorgan this.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:563
+
+static bool foldLoadsIterative(Instruction &I, const DataLayout &DL,
+                               TargetTransformInfo &TTI, AliasAnalysis *AA) {
----------------
Maybe replace the name of foldLoadsIterative with foldConsecutiveLoads and vice-versa. foldLoadsIterative doesn't really explain what this function is folding, and it's not really iterative.


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

https://reviews.llvm.org/D127392