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

[Biplob Mishra via Phabricator via llvm-commits]

bipmis added inline comments.


================
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)
----------------
dmgreen wrote:
> Are there any tests for case 2?
The case2 is not required as we are handling the pattern recursively. This also keeps the implementation simple.


================
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);
----------------
dmgreen wrote:
> Does it matter if it is a bitcast or a gep?
I think this may be needed, so that we fall through and evaluate further if instructions are only of these types. 


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:544
+  // are consecutive.
+  uint64_t ShiftDiff = IsBigEndian ? loadSize2 : loadSize1;
+  uint64_t PrevSize =
----------------
dmgreen wrote:
> We checked that loadSize1 == loadSize2 above.
Right. I have added additional comments on why this is needed.


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

https://reviews.llvm.org/D127392