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

[Jeroen Dobbelaere via Phabricator via llvm-commits]

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:
> 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).
I understand.

Again, the original idea is based on the full restrict version with the 'usage based' elimination of the scope declarations: when no actual pointer is based on the scope declaration associated with the restrict pointer, that declaration is not needed any more. A 'completely clean' version could combine this with going over _all_ instructions in the function and removing that scope from the metadata. With full restrict, that cost was considered to be too high. There, it is also possible to have multiple declarations using the same scope, but associated to different 'restrict pointers' sharing that scope.

IMHO, it does not make sense to only remove the declaration if the scope is not used at all. Once it is not used by either all `!alias.scope`, or all `!noalias`, it can be removed. A further refinement can indeed spend time in actually removing the unused/ineffective scope from the metadata. But I would rather do that as a followup change.



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

https://reviews.llvm.org/D95141