[PATCH] D91576: [MemCpyOpt] Correctly merge alias scopes during call slot optimization

[Di Mo via Phabricator via llvm-commits]

modimo added inline comments.


================
Comment at: llvm/lib/IR/Metadata.cpp:948
+
+  if (!std::includes(LargerDomain.begin(), LargerDomain.end(),
+                     SmallerDomain.begin(), SmallerDomain.end()))
----------------
jeroen.dobbelaere wrote:
> Two remarks:http://www.cplusplus.com/reference/algorithm/includes/ )
> 1. SmallPtrSet is not sorted, so it does not fulfill the preconditions for std::includes (See http://www.cplusplus.com/reference/algorithm/includes/ )
> 2. imho, this is still not a valid merging for !alias.scope (See external comment)
> 
> You probably want to do:
>   if (ADomains != BDomains)
>     return nullptr;
> 
> or take a real intersection of the domains, and then the union of the scopes belonging to the intersection of the domains.
> 
That's good to know about SmallPtrSet, I'll keep that in mind for the future. For (2) your approach is the correct one and I've updated the patch to match.


================
Comment at: llvm/test/Transforms/MemCpyOpt/callslot_badaa.ll:11
+;  !2 = distinct !{!2, !"callee1"}
+;  !3 = !{!1, !4, !5, !"callee0: %a"}
+;  !4 = distinct !{!4, !5, !"callee0: %a"}
----------------
tislam wrote:
> Regarding the alias.scope metadata `!3 = !{!1, !4, !5, !"callee0: %a"}`. Should this be `!3 = !{!1, !4}`? An alias.scope metadata should be a list of scope metadata and should not include any domain metadata (!5, for example).
Yes good catch. I regenerated the output with trunk which matches your statement. I suspect the top half and bottom half got out of sync at some point in the revisions.


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

https://reviews.llvm.org/D91576