-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[clang][CodeGen] Generate follow-up metadata for loops in correct format #131985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -22,20 +22,20 @@ using namespace clang::CodeGen; | |||||||||||||||||
using namespace llvm; | ||||||||||||||||||
|
||||||||||||||||||
MDNode * | ||||||||||||||||||
LoopInfo::createLoopPropertiesMetadata(ArrayRef<Metadata *> LoopProperties) { | ||||||||||||||||||
LoopInfo::createFollowupMetadata(const char *FollowupName, | ||||||||||||||||||
ArrayRef<llvm::Metadata *> LoopProperties) { | ||||||||||||||||||
LLVMContext &Ctx = Header->getContext(); | ||||||||||||||||||
SmallVector<Metadata *, 4> NewLoopProperties; | ||||||||||||||||||
NewLoopProperties.push_back(nullptr); | ||||||||||||||||||
NewLoopProperties.append(LoopProperties.begin(), LoopProperties.end()); | ||||||||||||||||||
|
||||||||||||||||||
MDNode *LoopID = MDNode::getDistinct(Ctx, NewLoopProperties); | ||||||||||||||||||
LoopID->replaceOperandWith(0, LoopID); | ||||||||||||||||||
return LoopID; | ||||||||||||||||||
SmallVector<Metadata *, 4> Args; | ||||||||||||||||||
Args.push_back(MDString::get(Ctx, FollowupName)); | ||||||||||||||||||
Args.append(LoopProperties.begin(), LoopProperties.end()); | ||||||||||||||||||
return MDNode::get(Ctx, Args); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
MDNode *LoopInfo::createPipeliningMetadata(const LoopAttributes &Attrs, | ||||||||||||||||||
ArrayRef<Metadata *> LoopProperties, | ||||||||||||||||||
bool &HasUserTransforms) { | ||||||||||||||||||
SmallVector<Metadata *, 4> | ||||||||||||||||||
LoopInfo::createPipeliningMetadata(const LoopAttributes &Attrs, | ||||||||||||||||||
ArrayRef<Metadata *> LoopProperties, | ||||||||||||||||||
bool &HasUserTransforms) { | ||||||||||||||||||
LLVMContext &Ctx = Header->getContext(); | ||||||||||||||||||
|
||||||||||||||||||
std::optional<bool> Enabled; | ||||||||||||||||||
|
@@ -44,23 +44,19 @@ MDNode *LoopInfo::createPipeliningMetadata(const LoopAttributes &Attrs, | |||||||||||||||||
else if (Attrs.PipelineInitiationInterval != 0) | ||||||||||||||||||
Enabled = true; | ||||||||||||||||||
|
||||||||||||||||||
SmallVector<Metadata *, 4> Args; | ||||||||||||||||||
Args.append(LoopProperties.begin(), LoopProperties.end()); | ||||||||||||||||||
|
||||||||||||||||||
if (Enabled != true) { | ||||||||||||||||||
SmallVector<Metadata *, 4> NewLoopProperties; | ||||||||||||||||||
if (Enabled == false) { | ||||||||||||||||||
NewLoopProperties.append(LoopProperties.begin(), LoopProperties.end()); | ||||||||||||||||||
NewLoopProperties.push_back( | ||||||||||||||||||
Args.push_back( | ||||||||||||||||||
MDNode::get(Ctx, {MDString::get(Ctx, "llvm.loop.pipeline.disable"), | ||||||||||||||||||
ConstantAsMetadata::get(ConstantInt::get( | ||||||||||||||||||
llvm::Type::getInt1Ty(Ctx), 1))})); | ||||||||||||||||||
LoopProperties = NewLoopProperties; | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Different from this PR, but I think we should set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is because by design There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about other transformations, e.g., vectorization? llvm-project/clang/lib/CodeGen/CGLoopInfo.cpp Lines 213 to 220 in ce8febb
I just looked for it and found an issue that might be caused by this. |
||||||||||||||||||
} | ||||||||||||||||||
return createLoopPropertiesMetadata(LoopProperties); | ||||||||||||||||||
return Args; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
SmallVector<Metadata *, 4> Args; | ||||||||||||||||||
Args.push_back(nullptr); | ||||||||||||||||||
Args.append(LoopProperties.begin(), LoopProperties.end()); | ||||||||||||||||||
|
||||||||||||||||||
if (Attrs.PipelineInitiationInterval > 0) { | ||||||||||||||||||
Metadata *Vals[] = { | ||||||||||||||||||
MDString::get(Ctx, "llvm.loop.pipeline.initiationinterval"), | ||||||||||||||||||
|
@@ -71,13 +67,11 @@ MDNode *LoopInfo::createPipeliningMetadata(const LoopAttributes &Attrs, | |||||||||||||||||
|
||||||||||||||||||
// No follow-up: This is the last transformation. | ||||||||||||||||||
|
||||||||||||||||||
MDNode *LoopID = MDNode::getDistinct(Ctx, Args); | ||||||||||||||||||
LoopID->replaceOperandWith(0, LoopID); | ||||||||||||||||||
HasUserTransforms = true; | ||||||||||||||||||
return LoopID; | ||||||||||||||||||
return Args; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
MDNode * | ||||||||||||||||||
SmallVector<Metadata *, 4> | ||||||||||||||||||
LoopInfo::createPartialUnrollMetadata(const LoopAttributes &Attrs, | ||||||||||||||||||
ArrayRef<Metadata *> LoopProperties, | ||||||||||||||||||
bool &HasUserTransforms) { | ||||||||||||||||||
|
@@ -108,11 +102,10 @@ LoopInfo::createPartialUnrollMetadata(const LoopAttributes &Attrs, | |||||||||||||||||
MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.unroll.disable"))); | ||||||||||||||||||
|
||||||||||||||||||
bool FollowupHasTransforms = false; | ||||||||||||||||||
MDNode *Followup = createPipeliningMetadata(Attrs, FollowupLoopProperties, | ||||||||||||||||||
FollowupHasTransforms); | ||||||||||||||||||
SmallVector<Metadata *, 4> Followup = createPipeliningMetadata( | ||||||||||||||||||
Attrs, FollowupLoopProperties, FollowupHasTransforms); | ||||||||||||||||||
|
||||||||||||||||||
SmallVector<Metadata *, 4> Args; | ||||||||||||||||||
Args.push_back(nullptr); | ||||||||||||||||||
Args.append(LoopProperties.begin(), LoopProperties.end()); | ||||||||||||||||||
|
||||||||||||||||||
// Setting unroll.count | ||||||||||||||||||
|
@@ -130,16 +123,14 @@ LoopInfo::createPartialUnrollMetadata(const LoopAttributes &Attrs, | |||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
if (FollowupHasTransforms) | ||||||||||||||||||
Args.push_back(MDNode::get( | ||||||||||||||||||
Ctx, {MDString::get(Ctx, "llvm.loop.unroll.followup_all"), Followup})); | ||||||||||||||||||
Args.push_back( | ||||||||||||||||||
createFollowupMetadata("llvm.loop.unroll.followup_all", Followup)); | ||||||||||||||||||
|
||||||||||||||||||
MDNode *LoopID = MDNode::getDistinct(Ctx, Args); | ||||||||||||||||||
LoopID->replaceOperandWith(0, LoopID); | ||||||||||||||||||
HasUserTransforms = true; | ||||||||||||||||||
return LoopID; | ||||||||||||||||||
return Args; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
MDNode * | ||||||||||||||||||
SmallVector<Metadata *, 4> | ||||||||||||||||||
LoopInfo::createUnrollAndJamMetadata(const LoopAttributes &Attrs, | ||||||||||||||||||
ArrayRef<Metadata *> LoopProperties, | ||||||||||||||||||
bool &HasUserTransforms) { | ||||||||||||||||||
|
@@ -170,11 +161,10 @@ LoopInfo::createUnrollAndJamMetadata(const LoopAttributes &Attrs, | |||||||||||||||||
MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.unroll_and_jam.disable"))); | ||||||||||||||||||
|
||||||||||||||||||
bool FollowupHasTransforms = false; | ||||||||||||||||||
MDNode *Followup = createPartialUnrollMetadata(Attrs, FollowupLoopProperties, | ||||||||||||||||||
FollowupHasTransforms); | ||||||||||||||||||
SmallVector<Metadata *, 4> Followup = createPartialUnrollMetadata( | ||||||||||||||||||
Attrs, FollowupLoopProperties, FollowupHasTransforms); | ||||||||||||||||||
|
||||||||||||||||||
SmallVector<Metadata *, 4> Args; | ||||||||||||||||||
Args.push_back(nullptr); | ||||||||||||||||||
Meinersbur marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
Args.append(LoopProperties.begin(), LoopProperties.end()); | ||||||||||||||||||
|
||||||||||||||||||
// Setting unroll_and_jam.count | ||||||||||||||||||
|
@@ -192,22 +182,18 @@ LoopInfo::createUnrollAndJamMetadata(const LoopAttributes &Attrs, | |||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
if (FollowupHasTransforms) | ||||||||||||||||||
Args.push_back(MDNode::get( | ||||||||||||||||||
Ctx, {MDString::get(Ctx, "llvm.loop.unroll_and_jam.followup_outer"), | ||||||||||||||||||
Followup})); | ||||||||||||||||||
Args.push_back(createFollowupMetadata( | ||||||||||||||||||
"llvm.loop.unroll_and_jam.followup_outer", Followup)); | ||||||||||||||||||
|
||||||||||||||||||
if (UnrollAndJamInnerFollowup) | ||||||||||||||||||
Args.push_back(MDNode::get( | ||||||||||||||||||
Ctx, {MDString::get(Ctx, "llvm.loop.unroll_and_jam.followup_inner"), | ||||||||||||||||||
UnrollAndJamInnerFollowup})); | ||||||||||||||||||
if (UnrollAndJamInnerFollowup.has_value()) | ||||||||||||||||||
Args.push_back(createFollowupMetadata( | ||||||||||||||||||
"llvm.loop.unroll_and_jam.followup_inner", *UnrollAndJamInnerFollowup)); | ||||||||||||||||||
|
||||||||||||||||||
MDNode *LoopID = MDNode::getDistinct(Ctx, Args); | ||||||||||||||||||
LoopID->replaceOperandWith(0, LoopID); | ||||||||||||||||||
HasUserTransforms = true; | ||||||||||||||||||
return LoopID; | ||||||||||||||||||
return Args; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
MDNode * | ||||||||||||||||||
SmallVector<Metadata *, 4> | ||||||||||||||||||
LoopInfo::createLoopVectorizeMetadata(const LoopAttributes &Attrs, | ||||||||||||||||||
ArrayRef<Metadata *> LoopProperties, | ||||||||||||||||||
bool &HasUserTransforms) { | ||||||||||||||||||
|
@@ -244,11 +230,10 @@ LoopInfo::createLoopVectorizeMetadata(const LoopAttributes &Attrs, | |||||||||||||||||
MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.isvectorized"))); | ||||||||||||||||||
|
||||||||||||||||||
bool FollowupHasTransforms = false; | ||||||||||||||||||
MDNode *Followup = createUnrollAndJamMetadata(Attrs, FollowupLoopProperties, | ||||||||||||||||||
FollowupHasTransforms); | ||||||||||||||||||
SmallVector<Metadata *, 4> Followup = createUnrollAndJamMetadata( | ||||||||||||||||||
Attrs, FollowupLoopProperties, FollowupHasTransforms); | ||||||||||||||||||
|
||||||||||||||||||
SmallVector<Metadata *, 4> Args; | ||||||||||||||||||
Args.push_back(nullptr); | ||||||||||||||||||
Args.append(LoopProperties.begin(), LoopProperties.end()); | ||||||||||||||||||
|
||||||||||||||||||
// Setting vectorize.predicate when it has been specified and vectorization | ||||||||||||||||||
|
@@ -315,17 +300,14 @@ LoopInfo::createLoopVectorizeMetadata(const LoopAttributes &Attrs, | |||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
if (FollowupHasTransforms) | ||||||||||||||||||
Args.push_back(MDNode::get( | ||||||||||||||||||
Ctx, | ||||||||||||||||||
{MDString::get(Ctx, "llvm.loop.vectorize.followup_all"), Followup})); | ||||||||||||||||||
Args.push_back( | ||||||||||||||||||
createFollowupMetadata("llvm.loop.vectorize.followup_all", Followup)); | ||||||||||||||||||
|
||||||||||||||||||
MDNode *LoopID = MDNode::getDistinct(Ctx, Args); | ||||||||||||||||||
LoopID->replaceOperandWith(0, LoopID); | ||||||||||||||||||
HasUserTransforms = true; | ||||||||||||||||||
return LoopID; | ||||||||||||||||||
return Args; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
MDNode * | ||||||||||||||||||
SmallVector<Metadata *, 4> | ||||||||||||||||||
LoopInfo::createLoopDistributeMetadata(const LoopAttributes &Attrs, | ||||||||||||||||||
ArrayRef<Metadata *> LoopProperties, | ||||||||||||||||||
bool &HasUserTransforms) { | ||||||||||||||||||
|
@@ -352,11 +334,10 @@ LoopInfo::createLoopDistributeMetadata(const LoopAttributes &Attrs, | |||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
bool FollowupHasTransforms = false; | ||||||||||||||||||
MDNode *Followup = | ||||||||||||||||||
SmallVector<Metadata *, 4> Followup = | ||||||||||||||||||
createLoopVectorizeMetadata(Attrs, LoopProperties, FollowupHasTransforms); | ||||||||||||||||||
|
||||||||||||||||||
SmallVector<Metadata *, 4> Args; | ||||||||||||||||||
Args.push_back(nullptr); | ||||||||||||||||||
Args.append(LoopProperties.begin(), LoopProperties.end()); | ||||||||||||||||||
|
||||||||||||||||||
Metadata *Vals[] = {MDString::get(Ctx, "llvm.loop.distribute.enable"), | ||||||||||||||||||
|
@@ -366,19 +347,17 @@ LoopInfo::createLoopDistributeMetadata(const LoopAttributes &Attrs, | |||||||||||||||||
Args.push_back(MDNode::get(Ctx, Vals)); | ||||||||||||||||||
|
||||||||||||||||||
if (FollowupHasTransforms) | ||||||||||||||||||
Args.push_back(MDNode::get( | ||||||||||||||||||
Ctx, | ||||||||||||||||||
{MDString::get(Ctx, "llvm.loop.distribute.followup_all"), Followup})); | ||||||||||||||||||
Args.push_back( | ||||||||||||||||||
createFollowupMetadata("llvm.loop.distribute.followup_all", Followup)); | ||||||||||||||||||
|
||||||||||||||||||
MDNode *LoopID = MDNode::getDistinct(Ctx, Args); | ||||||||||||||||||
LoopID->replaceOperandWith(0, LoopID); | ||||||||||||||||||
HasUserTransforms = true; | ||||||||||||||||||
return LoopID; | ||||||||||||||||||
return Args; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
MDNode *LoopInfo::createFullUnrollMetadata(const LoopAttributes &Attrs, | ||||||||||||||||||
ArrayRef<Metadata *> LoopProperties, | ||||||||||||||||||
bool &HasUserTransforms) { | ||||||||||||||||||
SmallVector<Metadata *, 4> | ||||||||||||||||||
LoopInfo::createFullUnrollMetadata(const LoopAttributes &Attrs, | ||||||||||||||||||
ArrayRef<Metadata *> LoopProperties, | ||||||||||||||||||
bool &HasUserTransforms) { | ||||||||||||||||||
LLVMContext &Ctx = Header->getContext(); | ||||||||||||||||||
|
||||||||||||||||||
std::optional<bool> Enabled; | ||||||||||||||||||
|
@@ -400,20 +379,17 @@ MDNode *LoopInfo::createFullUnrollMetadata(const LoopAttributes &Attrs, | |||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
SmallVector<Metadata *, 4> Args; | ||||||||||||||||||
Args.push_back(nullptr); | ||||||||||||||||||
Args.append(LoopProperties.begin(), LoopProperties.end()); | ||||||||||||||||||
Args.push_back(MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.unroll.full"))); | ||||||||||||||||||
|
||||||||||||||||||
// No follow-up: there is no loop after full unrolling. | ||||||||||||||||||
// TODO: Warn if there are transformations after full unrolling. | ||||||||||||||||||
|
||||||||||||||||||
MDNode *LoopID = MDNode::getDistinct(Ctx, Args); | ||||||||||||||||||
LoopID->replaceOperandWith(0, LoopID); | ||||||||||||||||||
HasUserTransforms = true; | ||||||||||||||||||
return LoopID; | ||||||||||||||||||
return Args; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
MDNode *LoopInfo::createMetadata( | ||||||||||||||||||
SmallVector<Metadata *, 4> LoopInfo::createMetadata( | ||||||||||||||||||
const LoopAttributes &Attrs, | ||||||||||||||||||
llvm::ArrayRef<llvm::Metadata *> AdditionalLoopProperties, | ||||||||||||||||||
bool &HasUserTransforms) { | ||||||||||||||||||
|
@@ -579,8 +555,8 @@ void LoopInfo::finish() { | |||||||||||||||||
MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.isvectorized"))); | ||||||||||||||||||
|
||||||||||||||||||
bool InnerFollowupHasTransform = false; | ||||||||||||||||||
MDNode *InnerFollowup = createMetadata(AfterJam, BeforeLoopProperties, | ||||||||||||||||||
InnerFollowupHasTransform); | ||||||||||||||||||
SmallVector<Metadata *, 4> InnerFollowup = createMetadata( | ||||||||||||||||||
AfterJam, BeforeLoopProperties, InnerFollowupHasTransform); | ||||||||||||||||||
if (InnerFollowupHasTransform) | ||||||||||||||||||
Parent->UnrollAndJamInnerFollowup = InnerFollowup; | ||||||||||||||||||
} | ||||||||||||||||||
|
@@ -589,7 +565,14 @@ void LoopInfo::finish() { | |||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
bool HasUserTransforms = false; | ||||||||||||||||||
LoopID = createMetadata(CurLoopAttr, {}, HasUserTransforms); | ||||||||||||||||||
SmallVector<Metadata *, 4> Properties = | ||||||||||||||||||
createMetadata(CurLoopAttr, {}, HasUserTransforms); | ||||||||||||||||||
SmallVector<Metadata *, 4> Args; | ||||||||||||||||||
Args.push_back(nullptr); | ||||||||||||||||||
Args.append(Properties.begin(), Properties.end()); | ||||||||||||||||||
LoopID = MDNode::getDistinct(Ctx, Args); | ||||||||||||||||||
LoopID->replaceOperandWith(0, LoopID); | ||||||||||||||||||
|
||||||||||||||||||
TempLoopID->replaceAllUsesWith(LoopID); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line was not executed when
Enabled == std::nullop
, sollvm.mustprogress
([[MP]]
) fromLoopProperties
is never added. Should have been added unconditionally.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
Enabled == std::nullopt
,LoopProperties
was used as is, notNewProperties
. So I think the cause is elsewhere. Anyway, it's enough to know thatllvm.mustprogress
should be appended unconditionally, thanks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle, LoopVectorize should know that if the original loop had a progress guarantee, then the vectorized loop will as well, so it should add
llvm.loop.must_progress
no matter what. I don't think it currently does.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is completely correct. What I didn't understand is, why the followup metadata of
LOOP_6
(FOLLOW_VECTOR_6
) didn't havellvm.mustprogress
before this patch, but now it (FOLLOWUP_VECTOR_3
) does. I investigated a little deeper and found the cause;FOLLOWUP_VECTOR_6
actually hadmustprogress
(?!). That is, the test passed for both of the following directives.Maybe FileCheck has a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same thing seems to happen elsewhere, e.g.
LOOP_6
actually hasvectorize.enable
but is not included in the CHECK directive.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is due to the regex
.*
being too greedy, so e.g.![[ISVECTORIZED:.*]]
consumes multiple metadata nodes. A better one would be[_a-zA-Z0-9.]+
. In principle![[ISVECTORIZED]] = {!"llvm.loop.isvectorized"}
(orUNROLL_8
) should be verified somewhere, which would then fail if it matched more than one node.IMHO there are lots of problems with FileCheck on LLVM-IR, and this is just one of them. Another one is that by default
CHECK: pet store
will matchcarpet store
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense! I got it, thank you!
Ugh, that's a tricky problem.