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

[Jeroen Dobbelaere via Phabricator via llvm-commits]

jeroen.dobbelaere added a comment.

About !alias.scope and !noalias (also see https://llvm.org/docs/LangRef.html#noalias-and-alias-scope-metadata):

- 'Scopes in one domain don't affect scopes in other domains' -> this is used during inlining
- The 'alias.scope' indicates for an instruction A will not alias with another instruction B IF, for a domain, the alias.scopes for A belonging to that domain are a subset of the 'noalias' scopes of instruction B.

Because of that:  when merging metadata at the same level (merging two `store` instructions for example), 
you can only keep the domains that are in both instructions valid. For those, you must take the union of the scopes.

Examples: (vXdY  : variable X in Domain Y)

a) same domain(s):

  store1: alias.scope v1d1
  store2: alias.scope v2d1
  =>
  store_merged: alias.scope  v1d1, v2d2

b) Disjunct domains

  store1 : alias.scope  v1d1
  store2 : alias.scope  v2d2
  =>
  store_merged:  NO alias.scope

c) partially overlapping (subset):

  store1: alias.scope v1d1
  store2: alias.scope v2d1, v3d2
  =>
  store_merged: alias.scope v1d1, v2d1

d) partially overlapping:

  store1: alias.scope v1d1, v2d2
  store2: alias.scope v3d2, v4d3
  =>
  store_merged: alias.scope v2d2, v3d2

NOTE: why is concatenate not valid ?

Take example (b). Suppose you have a:

  store3 : noalias  v1d1

now, `store1` and `store3` do not alias.   `store2` and `store3` might alias.
The merger `store_merged` might alias with `store3`. So, it is not valid to just take the union of  the scopes of `store1` and `store2`.



================
Comment at: llvm/lib/IR/Metadata.cpp:948
+
+  if (!std::includes(LargerDomain.begin(), LargerDomain.end(),
+                     SmallerDomain.begin(), SmallerDomain.end()))
----------------
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.



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

https://reviews.llvm.org/D91576