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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:642
+  NewLoad->takeName(LI1);
+  copyMetadataForLoad(*NewLoad, *LI1);
+  Builder.SetInsertPoint(LI1);
----------------
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.


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

https://reviews.llvm.org/D127392