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

[Dave Green via Phabricator via llvm-commits]

dmgreen accepted this revision.
dmgreen added a comment.
This revision is now accepted and ready to land.

Thanks for the the changes - as far as I understand, this LGTM now. Thanks for working through the details.

Any other comments?



================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:642
+  NewLoad->takeName(LI1);
+  copyMetadataForLoad(*NewLoad, *LI1);
+  Builder.SetInsertPoint(LI1);
----------------
bipmis wrote:
> 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.
I'm a little surprised that if two the tbaa info are the same, we can't use the same on the result node. I think using concat sounds sensible though. I suspect in practice we will often be combining char in any case.


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

https://reviews.llvm.org/D127392