[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:
> > > 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. 
> It protects us where there are thousands of instructions in the block, just to be safe for degenerate cases. If we expect the maximum pattern to be (load+zext+shift+or) * i8->i64, so 4*8, then a limit of 64 instructions sounds fine.
Sounds right. Will implement the same with the 64 instruction limit. Thanks.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:642
+  NewLoad->takeName(LI1);
+  copyMetadataForLoad(*NewLoad, *LI1);
+  Builder.SetInsertPoint(LI1);
----------------
nikic wrote:
> bipmis wrote:
> > 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);
> > 
> > ```
> Note that you can't simply take the AATags from one load, they have to be merged appropriately. I believe for this specific case you need the `AAMDNodes::concat()` method, because you are merging loads from different non-overlapping locations.
Agreed. Thanks for this.
I think we are better off dropping the metadata at this point.


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

https://reviews.llvm.org/D127392