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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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())
----------------
jeroen.dobbelaere wrote:
> 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.
> 
At least longer term, I would like that every scope use also has a corresponding decl. With that in mind, I don't really like the idea of leading behind "dangling" scope references, even if it is technically correct.

If it doesn't lose much practical optimization power, I would prefer it if we only dropped decls for scopes that are used in neither alias.scope nor noalias. But if this is a significant restriction, then I think your current approach of looking at alias.scope is fine (as it is annotated to loads, and those are much more likely to be eliminated than stores).


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

https://reviews.llvm.org/D95141