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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

The logic here looks sounds to me.



================
Comment at: llvm/include/llvm/Transforms/Utils/Cloning.h:295-297
+/// Find back the 'llvm.experimental.noalias.scope.decl' intrinsics in the
+/// specified basic blocks and extract their scope. This are candidates for
+/// duplication when cloning.
----------------



================
Comment at: llvm/include/llvm/Transforms/Utils/Cloning.h:300
+    ArrayRef<BasicBlock *> BBs,
+    SmallVectorImpl<MetadataAsValue *> &out_NoAliasDeclScopes);
+
----------------
I don't think we use this kind of `out_` style anywhere.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:1031
+        I->setOperand(opI, MvIt->second);
+      }
+    }
----------------
nit: No braces here.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:1040
+      for (auto &MDOp : CSNoAlias->operands()) {
+        if (MDNode *MD = dyn_cast_or_null<MDNode>(MDOp)) {
+          auto MdIt = ClonedScopes.find(MD);
----------------
Can operands be null?


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:1052
+        I->setMetadata(MD_ID, MDNode::get(Context, NewScopeList));
+      }
+    }
----------------
nit: No braces here.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:1076
+      adaptNoAliasScopes(&I, ClonedScopes, ClonedMVScopes, Context);
+    }
+  }
----------------
nit: No braces here.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:1083
+    SmallVectorImpl<MetadataAsValue *> &out_NoAliasDeclScopes) {
+  for (auto BB : BBs) {
+    for (Instruction &I : *BB) {
----------------
nit: `auto *` or `BasicBlock *`.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:86
+                        cl::desc("Use the llvm.experimental.noalias.scope.decl "
+                                 "intrinsic during inlining."));
 
----------------
These changes should have probably ended up in D93040.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:591
+  SmallVector<MetadataAsValue *, 6> LoopLocalNoAliasDeclScopes;
+  llvm::identifyNoAliasScopesToClone(L->getBlocks(),
+                                     LoopLocalNoAliasDeclScopes);
----------------
`llvm::` is presumably unnecessary here.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:691
+      // Phase3: after cloning, replace the metadata with the corrected version
+      // for both memory instructions and noalias intrinsics
+      std::string ext = (Twine("It") + Twine(It)).str();
----------------
With the implementation factored out the "phase 2" and "phase 3" comments look a bit out of place.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:694
+      cloneAndAdaptNoAliasScopes(LoopLocalNoAliasDeclScopes, NewBlocks,
+                                 Header->getContext(), ext);
+    }
----------------
Possible the `ext` parameter should be `Twine` rather than `StringRef`. But I'm not really familiar with our string API conventions.


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

https://reviews.llvm.org/D92887