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

[Biplob Mishra via Phabricator via llvm-commits]

bipmis marked 7 inline comments as done.
bipmis added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:567
+    if (++NumScanned > MaxInstrsToScan)
+      return false;
+  }
----------------
nikic wrote:
> This check should come first, otherwise you don't count store instructions.
So it would count stores which do not alias. For the case it aliases we terminate anyways and the count update wont matter?


================
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)
----------------
nikic wrote:
> 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.
Right. The shift condition will prevent it from merging. 
But we do not want to combine loads smaller than a byte. Updated checks.


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

https://reviews.llvm.org/D127392