[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:565
+  MemoryLocation Loc = MemoryLocation::get(LI2);
+  for (Instruction &Inst : make_range(LI1->getIterator(), LI2->getIterator())) {
+    if (Inst.mayWriteToMemory() && isModSet(AA->getModRefInfo(&Inst, Loc)))
----------------
dmgreen wrote:
> 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?
In the current implementation worst case limit could be all instructions in a BB. What is the issue with this?
For the test case load64_farLoads() it does fine with a limit of 35. 


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:642
+  NewLoad->takeName(LI1);
+  copyMetadataForLoad(*NewLoad, *LI1);
+  Builder.SetInsertPoint(LI1);
----------------
dmgreen wrote:
> 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?
This being a specific scenario of the pattern match and looking for an or-load chain, I dont think performance should be a big concern. Depends on the end use of the merged load. 

What I am seeing in most cases is that they try to retain atleast the AATags. 

```
if (AATags)
      NewVal->setAAMetadata(AATags);

```


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

https://reviews.llvm.org/D127392