[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:642
+  NewLoad->takeName(LI1);
+  copyMetadataForLoad(*NewLoad, *LI1);
+  Builder.SetInsertPoint(LI1);
----------------
bipmis wrote:
> 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.
The concat method leaves the tbaa blank so maybe we may want to drop the Metadata  altogether? 
Currently the ‘noalias’ and ‘alias.scope’ Metadata will be concatenated from 
AAMDNodes.


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

https://reviews.llvm.org/D127392