[PATCH] D95141: [InstCombine] Remove unused llvm.experimental.noalias.scope.decl

[Jeroen Dobbelaere via Phabricator via llvm-commits]

jeroen.dobbelaere added a subscriber: hfinkel.
jeroen.dobbelaere added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3753
+  void analyse(Instruction *I) {
+    if (auto *MD = I->getMetadata(LLVMContext::MD_alias_scope))
+      for (auto &MDOperand : cast<MDNode>(MD)->operands())
----------------
nikic wrote:
> Why only MD_alias_scope and not also MD_noalias?
`MD_noalias` is telling what scopes (variables) are 'active'/'visible' for the memory instruction, but to which it will not alias.
`MD_alias_scope`is telling what 'variable(s)' is/are associated to the memory instruction (to which it _will_ alias).

A no-aliasing relationship between two instructions can be deduced by checking if the `!alias.scope` of one instruction is a subset of the `!noalias` of the other instruction (also taking into account the 'domain').

Once a scope is not any more used in `!alias.scope` (aka, those memory instructions have been optimized away, or they lost the metadata), the location of that scope declaration has no influence any more and can be omitted.
(With the full restrict patches, this corresponds the usage count of the declaration becoming 0)

 Of course the same reasoning can be done for the `!noalias` metadata: once a declared scope is not used any more in `!noalias` you can come to the same conclusion and omit that declaration.

hmm..

My reasoning was that we are more likely to hit this with the '!alias.scope'. But a more complete solution can also track the `UsedNoAliasScopes`. If the declared scope is not found in one of the two, it (and its declaration) become useless and we can omit it. Removing the declaration is easy and safe. I am not sure if removing the metadata associated with that scope is also easy and safe to do. Shared metadata between functions could make it tricky, although that should only happen in testcases as far as I am aware. 

So, the short answer is: focusing on `MD_alias_scope` is likely the most effective (and corresponds to how the full restrict patches handle it)

But, extending the analysis to also track `MD_noalias` can provide a more complete solution, at a slightly higher cost.

@hfinkel, do you agree with my reasoning ?

@nikic, @jdoerfert shall I add the `UsedNoAliasScopes` tracking as well ?
Whatever we choose, some extra explanation why this is valid should also be added.



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

https://reviews.llvm.org/D95141