[PATCH] D93039: Introduce llvm.noalias.decl intrinsic

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D93039#2486994 <https://reviews.llvm.org/D93039#2486994>, @jeroen.dobbelaere wrote:

> Another observation/question I have is about possible optimizations for omitting the `@llvm.experimental.noalias.scope.decl`: In the full restrict patches, the declarations are thrown away when there are no users any more. The equivalent with the implementation here, is that we can throw away the declaration if the scope it declares is not used in any `!alias.scope` metadata. Would this be something that is easy (and fast) to check ?

As far as I can tell metadata does not have use lists, so I don't think this can be done without a full scan of the function to collect used scopes.



================
Comment at: llvm/docs/LangRef.rst:19596
+noalias scope is declared. When the intrinsic is duplicated, also a decision
+must be made about the scope: depending on the reason of the duplication,
+sometimes the scope must be duplicated as well.
----------------
Same as below "also a decision must be made" -> "a decision must also be made"


================
Comment at: llvm/docs/LangRef.rst:19608
+Semantics:
+""""""""""
+
----------------
The domination rule enforced by the verifier should be mentioned here as well.


================
Comment at: llvm/lib/IR/Verifier.cpp:5535
+           "Not a llvm.experimental.noalias.scope.decl ?");
+    auto *ScopeListMV = dyn_cast<MetadataAsValue>(
+        II->getOperand(Intrinsic::NoAliasScopeDeclScopeArg));
----------------
`const auto *`?


================
Comment at: llvm/lib/IR/Verifier.cpp:5548
+  }
+
+  // Now sort the intrinsics based on the scope MDNode so that declarations of
----------------
It might make sense to temporarily put the domination check behind a default disabled `cl::opt`. Unless we want to land everything at the same time (which I wouldn't recommend) there will be an intermediate time where the verifier check will not succeed.


================
Comment at: llvm/lib/IR/Verifier.cpp:5552
+  auto GetScope = [](IntrinsicInst *II) {
+    auto LhsScopeListMV = cast<MetadataAsValue>(
+        II->getOperand(Intrinsic::NoAliasScopeDeclScopeArg));
----------------
`auto *`


================
Comment at: llvm/lib/IR/Verifier.cpp:5561
+
+  std::sort(NoAliasScopeDecls.begin(), NoAliasScopeDecls.end(), Compare);
+
----------------
`llvm::sort(NoAliasScopeDecls, Compare)`


================
Comment at: llvm/lib/IR/Verifier.cpp:5588
+  }
+}
+
----------------
Can you please also add a test for this in `llvm/test/Verifier`?


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

https://reviews.llvm.org/D93039