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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:560
+  uint64_t LoadSize2 = DL.getTypeStoreSizeInBits(LI2->getType());
+  if ((Load1Ptr != Load2Ptr) || (LoadSize1 != LoadSize2))
+    return false;
----------------
Drop these extra brackets


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:565
+  MemoryLocation Loc = MemoryLocation::get(LI2);
+  for (Instruction &Inst : make_range(LI1->getIterator(), LI2->getIterator())) {
+    if (Inst.mayWriteToMemory() && isModSet(AA->getModRefInfo(&Inst, Loc)))
----------------
Should this have a limit on the number of instructions?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:575
+  // Find Shifts values.
+  const APInt *Temp;
+  uint64_t Shift1 = 0, Shift2 = 0;
----------------
Is there a test for an i128 version of the fold?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:642
+  NewLoad->takeName(LI1);
+  copyMetadataForLoad(*NewLoad, *LI1);
+  Builder.SetInsertPoint(LI1);
----------------
Is all the metadata on the old instruction (like MD_range) always valid on the new load?


================
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);
----------------
bipmis wrote:
> 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. 
What is it you mean by fall though? If stripAndAccumulateConstantOffsets could give us an Offset, it seems like we should just always call it and have it do what it can. It will return the original pointer if it couldn't do anything useful.
It may be better to keep Offset1/Offset2 as APInt too. It would help if the pointers were > 64bits.


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

https://reviews.llvm.org/D127392