[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:565
+  MemoryLocation Loc = MemoryLocation::get(LI2);
+  for (Instruction &Inst : make_range(LI1->getIterator(), LI2->getIterator())) {
+    if (Inst.mayWriteToMemory() && isModSet(AA->getModRefInfo(&Inst, Loc)))
----------------
bipmis wrote:
> dmgreen wrote:
> > Should this have a limit on the number of instructions?
> Some tests like load64_farLoads() which have wider instruction gap b/w loads may result in partial combine(when tried with 16). I can possibly go for a bigger limit or can keep the limit on the actual instructions b/w 2 loads.
Using what limit?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:642
+  NewLoad->takeName(LI1);
+  copyMetadataForLoad(*NewLoad, *LI1);
+  Builder.SetInsertPoint(LI1);
----------------
dmgreen wrote:
> Is all the metadata on the old instruction (like MD_range) always valid on the new load?
What happens if new metadata gets added in the future, that isn't valid?

Is it better to just drop all the metadata? Or is that too likely to be worse for performance?


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

https://reviews.llvm.org/D127392