[PATCH] D92887: [LoopUnroll] Use llvm.experimental.noalias.scope.decl for duplicating noalias metadata as needed
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 20 13:17:08 PST 2021
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
More information about the llvm-commits
mailing list