-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[licm] clone MD_prof
when hoisting conditional branch
#152232
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
[licm] clone MD_prof
when hoisting conditional branch
#152232
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-analysis Author: Mircea Trofin (mtrofin) ChangesThe metadata information for the hoisted conditional branch should be copied from the original branch, not from the current terminator of the block it's hoisted to. Note that debug info was previously copied by (This was identified through Full diff: https://github.com/llvm/llvm-project/pull/152232.diff 3 Files Affected:
diff --git a/llvm/lib/Analysis/BranchProbabilityInfo.cpp b/llvm/lib/Analysis/BranchProbabilityInfo.cpp
index 671e023415a6e..1467bcd61ec8b 100644
--- a/llvm/lib/Analysis/BranchProbabilityInfo.cpp
+++ b/llvm/lib/Analysis/BranchProbabilityInfo.cpp
@@ -59,6 +59,13 @@ static cl::opt<std::string> PrintBranchProbFuncName(
cl::desc("The option to specify the name of the function "
"whose branch probability info is printed."));
+// Flag used for an ablation performance test, Issue #147390. Placing it here
+// because referencing Analysis should be feasible from anywhere. Will be
+// removed after the ablation test.
+cl::opt<bool> DisableProfilingInfoCorrectPropagation(
+ "profcheck-disable-fixes", cl::Hidden, cl::init(false),
+ cl::desc("Temporary flag, will be used for an ablation test"));
+
INITIALIZE_PASS_BEGIN(BranchProbabilityInfoWrapperPass, "branch-prob",
"Branch Probability Analysis", false, true)
INITIALIZE_PASS_DEPENDENCY(LoopInfoWrapperPass)
diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index c3f80f901a120..3a17e34573cc4 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -218,6 +218,8 @@ using PointersAndHasReadsOutsideSet =
static SmallVector<PointersAndHasReadsOutsideSet, 0>
collectPromotionCandidates(MemorySSA *MSSA, AliasAnalysis *AA, Loop *L);
+extern cl::opt<bool> DisableProfilingInfoCorrectPropagation;
+
namespace {
struct LoopInvariantCodeMotion {
bool runOnLoop(Loop *L, AAResults *AA, LoopInfo *LI, DominatorTree *DT,
@@ -857,9 +859,18 @@ class ControlFlowHoister {
}
// Now finally clone BI.
- ReplaceInstWithInst(
- HoistTarget->getTerminator(),
- BranchInst::Create(HoistTrueDest, HoistFalseDest, BI->getCondition()));
+ auto *NewBI =
+ BranchInst::Create(HoistTrueDest, HoistFalseDest, BI->getCondition());
+ ReplaceInstWithInst(HoistTarget->getTerminator(), NewBI);
+ // Copy all the metadata. In particular:
+ // - debug info (critical to Sample-based profiling) should be the same as
+ // the original branch, not that of HoistTarget->getTerminator(), which is
+ // what ReplaceInstWithInst would use.
+ // - md_prof should also come from the original branch - since the condition
+ // was hoisted, the branch probabilities shouldn't change.
+ if (!DisableProfilingInfoCorrectPropagation)
+ NewBI->copyMetadata(*BI);
+
++NumClonedBranches;
assert(CurLoop->getLoopPreheader() &&
diff --git a/llvm/test/Transforms/LICM/hoist-phi-metadata.ll b/llvm/test/Transforms/LICM/hoist-phi-metadata.ll
new file mode 100644
index 0000000000000..031a00d11bbf3
--- /dev/null
+++ b/llvm/test/Transforms/LICM/hoist-phi-metadata.ll
@@ -0,0 +1,46 @@
+; Test that hoising conditional branches copies over profiling metadata
+; RUN: opt -S -passes=licm -licm-control-flow-hoisting=1 %s -o - | FileCheck %s
+
+; CHECK-LABEL: @triangle_phi
+define void @triangle_phi(i32 %x, ptr %p) {
+; CHECK-LABEL: entry:
+; CHECK: %cmp1 = icmp sgt i32 %x, 0
+; CHECK: br i1 %cmp1, label %[[IF_LICM:.*]], label %[[THEN_LICM:.*]], !prof !0
+entry:
+ br label %loop
+
+; CHECK: [[IF_LICM]]:
+; CHECK: %add = add i32 %x, 1
+; CHECK: br label %[[THEN_LICM]]
+
+; CHECK: [[THEN_LICM]]:
+; CHECK: phi i32 [ %add, %[[IF_LICM]] ], [ %x, %entry ]
+; CHECK: store i32 %phi, ptr %p
+; CHECK: %cmp2 = icmp ne i32 %phi, 0
+; CHECK: br label %loop
+
+loop:
+ %cmp1 = icmp sgt i32 %x, 0
+ br i1 %cmp1, label %if, label %then, !prof !0
+
+if:
+ %add = add i32 %x, 1
+ br label %then
+
+; CHECK-LABEL: then:
+; CHECK: br i1 %cmp2, label %loop, label %end, !prof !1
+then:
+ %phi = phi i32 [ %add, %if ], [ %x, %loop ]
+ store i32 %phi, ptr %p
+ %cmp2 = icmp ne i32 %phi, 0
+ br i1 %cmp2, label %loop, label %end, !prof !1
+
+; CHECK-LABEL: end:
+end:
+ ret void
+}
+
+; CHECK: !0 = !{!"branch_weights", i32 5, i32 7}
+; CHECK: !1 = !{!"branch_weights", i32 13, i32 11}
+!0 = !{!"branch_weights", i32 5, i32 7}
+!1 = !{!"branch_weights", i32 13, i32 11}
|
@llvm/pr-subscribers-llvm-transforms Author: Mircea Trofin (mtrofin) ChangesThe metadata information for the hoisted conditional branch should be copied from the original branch, not from the current terminator of the block it's hoisted to. Note that debug info was previously copied by (This was identified through Full diff: https://github.com/llvm/llvm-project/pull/152232.diff 3 Files Affected:
diff --git a/llvm/lib/Analysis/BranchProbabilityInfo.cpp b/llvm/lib/Analysis/BranchProbabilityInfo.cpp
index 671e023415a6e..1467bcd61ec8b 100644
--- a/llvm/lib/Analysis/BranchProbabilityInfo.cpp
+++ b/llvm/lib/Analysis/BranchProbabilityInfo.cpp
@@ -59,6 +59,13 @@ static cl::opt<std::string> PrintBranchProbFuncName(
cl::desc("The option to specify the name of the function "
"whose branch probability info is printed."));
+// Flag used for an ablation performance test, Issue #147390. Placing it here
+// because referencing Analysis should be feasible from anywhere. Will be
+// removed after the ablation test.
+cl::opt<bool> DisableProfilingInfoCorrectPropagation(
+ "profcheck-disable-fixes", cl::Hidden, cl::init(false),
+ cl::desc("Temporary flag, will be used for an ablation test"));
+
INITIALIZE_PASS_BEGIN(BranchProbabilityInfoWrapperPass, "branch-prob",
"Branch Probability Analysis", false, true)
INITIALIZE_PASS_DEPENDENCY(LoopInfoWrapperPass)
diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index c3f80f901a120..3a17e34573cc4 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -218,6 +218,8 @@ using PointersAndHasReadsOutsideSet =
static SmallVector<PointersAndHasReadsOutsideSet, 0>
collectPromotionCandidates(MemorySSA *MSSA, AliasAnalysis *AA, Loop *L);
+extern cl::opt<bool> DisableProfilingInfoCorrectPropagation;
+
namespace {
struct LoopInvariantCodeMotion {
bool runOnLoop(Loop *L, AAResults *AA, LoopInfo *LI, DominatorTree *DT,
@@ -857,9 +859,18 @@ class ControlFlowHoister {
}
// Now finally clone BI.
- ReplaceInstWithInst(
- HoistTarget->getTerminator(),
- BranchInst::Create(HoistTrueDest, HoistFalseDest, BI->getCondition()));
+ auto *NewBI =
+ BranchInst::Create(HoistTrueDest, HoistFalseDest, BI->getCondition());
+ ReplaceInstWithInst(HoistTarget->getTerminator(), NewBI);
+ // Copy all the metadata. In particular:
+ // - debug info (critical to Sample-based profiling) should be the same as
+ // the original branch, not that of HoistTarget->getTerminator(), which is
+ // what ReplaceInstWithInst would use.
+ // - md_prof should also come from the original branch - since the condition
+ // was hoisted, the branch probabilities shouldn't change.
+ if (!DisableProfilingInfoCorrectPropagation)
+ NewBI->copyMetadata(*BI);
+
++NumClonedBranches;
assert(CurLoop->getLoopPreheader() &&
diff --git a/llvm/test/Transforms/LICM/hoist-phi-metadata.ll b/llvm/test/Transforms/LICM/hoist-phi-metadata.ll
new file mode 100644
index 0000000000000..031a00d11bbf3
--- /dev/null
+++ b/llvm/test/Transforms/LICM/hoist-phi-metadata.ll
@@ -0,0 +1,46 @@
+; Test that hoising conditional branches copies over profiling metadata
+; RUN: opt -S -passes=licm -licm-control-flow-hoisting=1 %s -o - | FileCheck %s
+
+; CHECK-LABEL: @triangle_phi
+define void @triangle_phi(i32 %x, ptr %p) {
+; CHECK-LABEL: entry:
+; CHECK: %cmp1 = icmp sgt i32 %x, 0
+; CHECK: br i1 %cmp1, label %[[IF_LICM:.*]], label %[[THEN_LICM:.*]], !prof !0
+entry:
+ br label %loop
+
+; CHECK: [[IF_LICM]]:
+; CHECK: %add = add i32 %x, 1
+; CHECK: br label %[[THEN_LICM]]
+
+; CHECK: [[THEN_LICM]]:
+; CHECK: phi i32 [ %add, %[[IF_LICM]] ], [ %x, %entry ]
+; CHECK: store i32 %phi, ptr %p
+; CHECK: %cmp2 = icmp ne i32 %phi, 0
+; CHECK: br label %loop
+
+loop:
+ %cmp1 = icmp sgt i32 %x, 0
+ br i1 %cmp1, label %if, label %then, !prof !0
+
+if:
+ %add = add i32 %x, 1
+ br label %then
+
+; CHECK-LABEL: then:
+; CHECK: br i1 %cmp2, label %loop, label %end, !prof !1
+then:
+ %phi = phi i32 [ %add, %if ], [ %x, %loop ]
+ store i32 %phi, ptr %p
+ %cmp2 = icmp ne i32 %phi, 0
+ br i1 %cmp2, label %loop, label %end, !prof !1
+
+; CHECK-LABEL: end:
+end:
+ ret void
+}
+
+; CHECK: !0 = !{!"branch_weights", i32 5, i32 7}
+; CHECK: !1 = !{!"branch_weights", i32 13, i32 11}
+!0 = !{!"branch_weights", i32 5, i32 7}
+!1 = !{!"branch_weights", i32 13, i32 11}
|
667dcb0
to
14815d4
Compare
llvm/lib/Transforms/Scalar/LICM.cpp
Outdated
// - md_prof should also come from the original branch - since the condition | ||
// was hoisted, the branch probabilities shouldn't change. | ||
if (!DisableProfilingInfoCorrectPropagation) | ||
NewBI->copyMetadata(*BI); |
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.
For debug information, it looks like ReplaceInstWithInst first copies over the wrong debug info, then we update it to the right one as part of this copyMetadata call? The hidden manipulation of some types of metadata seems like a design that could be improved.
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 code probably shouldn't be using ReplaceInstWithInst at all. The things it does are largely useless in this context.
Should probably do something along these lines?
auto *NewBI = BranchInst::Create(..., HoistTarget->getTerminator());
NewBI->copyMetadata(*BI);
HoistTarget->getTerminator()->eraseFromParent();
96a8b81
to
12346fa
Compare
// what ReplaceInstWithInst would use. | ||
// - md_prof should also come from the original branch - since the condition | ||
// was hoisted, the branch probabilities shouldn't change. | ||
if (!ProfcheckDisableMetadataFixes) |
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.
Control flow hoisting in LICM is not enabled by default, so I don't think this option is useful.
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.
For its temporary purpose, it's easier if I just control all these kinds of changes this way. I'll remove it after (basically I want to lump all fixes I can detect via profcheck, and it's OK if some aren't enabled anyway - just want to approximate the benefit)
12346fa
to
0494ce7
Compare
llvm/lib/Transforms/Scalar/LICM.cpp
Outdated
// Copy all the metadata. In particular: | ||
// - debug info (critical to Sample-based profiling) should be the same | ||
// as the original branch, not that of HoistTarget->getTerminator(), | ||
// which is what ReplaceInstWithInst would use. |
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.
Hm, looking at https://llvm.org/docs/HowToUpdateDebugInfo.html#when-to-preserve-an-instruction-location again, it specifically calls out LICM as a case where the debug location should be dropped, not preserved.
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.
Surprising / counter-intuitive, since the article does talk about SamplePGO. @vedantk who edited that last.
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.
We had a similar discussion about merging source locations and it's intended usage with AutoFDO. We find that retaining some location (though potentially misleading) is better for profile quality. See #129960 for discussion and updates to the section just below the linked one above where the guidance has been updated. We haven't updated the guidance for dropping source locations though this is probably a good time as any to have the conversation and make the change.
Edit: updated PR link to the correct one.
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.
Besides, in this case it wouldn't be misleading, for AutoFDO purposes either (or someone correct me if I'm missing something)
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.
Debug info usage of debuggers and SamplePGO is at odds here. Debuggers want this dropped, while SamplePGO does not. Our debug info policy is generally targeted towards debuggers.
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.
Filed #152767 to track the addition of a new flag and to update the documentation.
I don't think we should enable it by default for O2/O3 though. I received pushback even when suggesting we add it under -fdebug-info-for-profiling
. It should be purely opt-in especially for the cases where the location may be non-representable.
To keep this PR moving along can we leave the behaviour for debug information unchanged (add a TODO with the issue number) and only address the profile metadata being dropped?
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.
Oh, if there's precedent re. auto-enabling, sgtm.
Adding the flag and change in behavior is trivial, especially since (my earlier comment) we actually shouldn't just move all the metadata over. The documentation part of Issue #152767 can be addressed separately, imho.
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.
Agree the change here is small but we should do this as a separate change to solicit feedback from other debug info stakeholders.
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.
You'll still want to explicitly set the location to dropped here to not run afoul of verification (https://discourse.llvm.org/t/rfc-require-real-or-annotated-source-locations-on-all-instructions/86816).
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.
Ack. Kept the behavior we had previously for debuginfo
37ea48f
to
82b2df1
Compare
// FIXME: Flag used for an ablation performance test, Issue #147390. Placing it | ||
// here because referencing IR should be feasible from anywhere. Will be | ||
// removed after the ablation test. | ||
cl::opt<bool> ProfcheckDisableMetadataFixes( |
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.
moved it to IR
because... well, see the stacked PR (still draft because I am thinking where to actually put the test)
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 comment doesn't make sense anymore, but I'd leave it in IR
to avoid later churn.
82b2df1
to
9bea77f
Compare
9bea77f
to
ac134fb
Compare
cac2eba
to
b8aa221
Compare
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.
lgtm.
b8aa221
to
223404b
Compare
MD_prof
when hoisting conditional branch
223404b
to
871de1e
Compare
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 code change LGTM. For the option, I think I was wrong in saying that this needs to be in a header with LLVM_ABI. I see lots of other places that do cross-imports of options with just extern cl::opt<>
. @andrurogerz Is it okay to just have cl::opt<bool> ProfcheckDisableMetadataFixes
in Instructions.cpp and extern cl::opt<bool> ProfcheckDisableMetadataFixes
in LICM.cpp?
You will need |
@andrurogerz This only matters if we want to export the option from the DLL though, right? If it's just purely internal (but used across multiple files) it should be fine without annotations? We seems to have lots of options like this, |
@nikic yes, good point. I missed that it was internal only. You only need to annotate it for export if a test or other executable needs to reference the symbol. I see that it is declared in a public header, though, can it move to a private header? I realize it is referenced cross-library so perhaps not. |
I'm assuming in the Windows case, there's a LLVM.dll that we're talking about here? i.e. the component boundary isn't IR vs Transforms, it's more coarse-grained. |
871de1e
to
8ed35ac
Compare
Yeah, exactly. |
The profiling - related metadata information for the hoisted conditional branch should be copied from the original branch, not from the current terminator of the block it's hoisted to.
The patch adds a way to disable the fix just so we can do an ablation test, after which the flag will be removed. The same flag will be reused for other similar fixes.
(This was identified through
profcheck
(see Issue #147390), and this PR addresses most of the test failures (when running under profcheck) underTransforms/LICM
.)