[PATCH] D92887: [LoopUnroll] Use llvm.experimental.noalias.scope.decl for duplicating noalias metadata as needed

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/include/llvm/Transforms/Utils/Cloning.h:271
 
+/// Find back the 'llvm.experimental.noalias.scope.decl' intrinsics in the
+/// specified basic blocks and extract their scope. These are candidates for
----------------
nit: "back" is too much here.


================
Comment at: llvm/include/llvm/Transforms/Utils/Cloning.h:301
+/// NewBlocks basicblocks to the cloned versions.
+/// 'Ext' will be added to the duplicate scope names
+void cloneAndAdaptNoAliasScopes(ArrayRef<MetadataAsValue *> NoAliasDeclScopes,
----------------
nit: Missing period at end of sentence.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:901
+      if (MDNode *MD = dyn_cast<MDNode>(MDOperand)) {
+        llvm::AliasScopeNode SNANode(MD);
+
----------------
nit: Presumably unnecessary `llvm::` prefix.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:927
+  // MetadataAsValue will always be replaced !
+  for (int opI = 0, opIEnd = I->getNumOperands(); opI < opIEnd; ++opI) {
+    if (MetadataAsValue *MV = dyn_cast<MetadataAsValue>(I->getOperand(opI))) {
----------------
I believe you should be able to write this as `for (Use &U : I->operands())` and then use `U.set()` to replace the operand.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:930
+      auto MvIt = ClonedMVScopes.find(MV);
+      if (MvIt != ClonedMVScopes.end())
+        I->setOperand(opI, MvIt->second);
----------------
Should be possible to use something like `if (MetadataAsValue *NewMV = ClonedMVScopes.lookup(MV))` here.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:937
+    if (const MDNode *CSNoAlias = I->getMetadata(MD_ID)) {
+      bool needsReplacement = false;
+      SmallVector<Metadata *, 8> NewScopeList;
----------------
nit: Should be `NeedsReplacement`.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:942
+          auto MdIt = ClonedScopes.find(MD);
+          if (MdIt != ClonedScopes.end()) {
+            NewScopeList.push_back(MdIt->second);
----------------
Here again, something like `MDNode *NewMD = ClonedScopes.lookup(MD))` should be possible.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:593
 
+  // Phase1: Identify what noalias metadata is inside the loop: if it is inside
+  // the loop, the associated metadata must be cloned for each iteration.
----------------
nit: The "Phase1" here no longer makes sense (was dropped below).


================
Comment at: llvm/test/Transforms/LoopUnroll/noalias.ll:62
+; CHECK: !7 = !{!8}
+; CHECK: !8 = distinct !{!8, !2, !"It3"}
----------------
It might be worthwhile to also add a test with noalias.scope.decl outside the loop, that shows that no scopes are cloned in that case.


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

https://reviews.llvm.org/D92887