paulkirth created this revision.
paulkirth added a reviewer: tejohnson.
Herald added subscribers: ormris, okura, kuter, hiraditya.
Herald added a project: All.
paulkirth requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a reviewer: sstefan1.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.
MisExpect needs to know if a branch weight intrinsic originates from an
llvm.expect intrinsic.
This patch allows us to track that provenance by adding a new
metadata type that can be moved in concert with the existing branch
weights. The new metadata is copied whenever branch weights are copied,
and is removed when new branch weights are added.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D131306
Files:
clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp
clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp
llvm/include/llvm/IR/FixedMetadataKinds.def
llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
llvm/lib/Transforms/IPO/Attributor.cpp
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
llvm/lib/Transforms/IPO/LowerTypeTests.cpp
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
llvm/lib/Transforms/Utils/Local.cpp
llvm/lib/Transforms/Utils/MisExpect.cpp
llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
Index: llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
===================================================================
--- llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
+++ llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
@@ -138,7 +138,7 @@
%conv1 = sext i32 %conv to i64
%expval = call i64 @llvm.expect.with.probability.i64(i64 %conv1, i64 0, double 8.000000e-01)
%tobool = icmp ne i64 %expval, 0
-; CHECK: !prof !1
+; CHECK: !prof !2
; CHECK-NOT: @llvm.expect.with.probability
br i1 %tobool, label %if.then, label %if.end
@@ -165,7 +165,7 @@
%tmp = load i32, i32* %x.addr, align 4
%conv = sext i32 %tmp to i64
%expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 2, double 8.000000e-01)
-; CHECK: !prof !2
+; CHECK: !prof !3
; CHECK-NOT: @llvm.expect.with.probability
switch i64 %expval, label %sw.epilog [
i64 1, label %sw.bb
@@ -194,7 +194,7 @@
%tmp = load i32, i32* %x.addr, align 4
%conv = sext i32 %tmp to i64
%expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.000000e-01)
-; CHECK: !prof !3
+; CHECK: !prof !4
; CHECK-NOT: @llvm.expect.with.probability
switch i64 %expval, label %sw.epilog [
i64 2, label %sw.bb
@@ -278,7 +278,7 @@
%t7 = call i64 @llvm.expect.with.probability.i64(i64 %t6, i64 0, double 8.000000e-01)
%t8 = icmp ne i64 %t7, 0
%t9 = select i1 %t8, i32 1, i32 2
-; CHECK: select{{.*}}, !prof !1
+; CHECK: select{{.*}}, !prof !2
ret i32 %t9
}
@@ -286,6 +286,6 @@
declare i1 @llvm.expect.with.probability.i1(i1, i1, double) nounwind readnone
; CHECK: !0 = !{!"branch_weights", i32 1717986918, i32 429496731}
-; CHECK: !1 = !{!"branch_weights", i32 429496731, i32 1717986918}
-; CHECK: !2 = !{!"branch_weights", i32 214748366, i32 214748366, i32 1717986918}
-; CHECK: !3 = !{!"branch_weights", i32 1717986918, i32 214748366, i32 214748366}
+; CHECK: !2 = !{!"branch_weights", i32 429496731, i32 1717986918}
+; CHECK: !3 = !{!"branch_weights", i32 214748366, i32 214748366, i32 1717986918}
+; CHECK: !4 = !{!"branch_weights", i32 1717986918, i32 214748366, i32 214748366}
Index: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
===================================================================
--- llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
+++ llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
@@ -138,7 +138,7 @@
%conv1 = sext i32 %conv to i64
%expval = call i64 @llvm.expect.i64(i64 %conv1, i64 0)
%tobool = icmp ne i64 %expval, 0
-; CHECK: !prof !1
+; CHECK: !prof !2
; CHECK-NOT: @llvm.expect
br i1 %tobool, label %if.then, label %if.end
@@ -165,7 +165,7 @@
%tmp = load i32, i32* %x.addr, align 4
%conv = sext i32 %tmp to i64
%expval = call i64 @llvm.expect.i64(i64 %conv, i64 2)
-; CHECK: !prof !2
+; CHECK: !prof !3
; CHECK-NOT: @llvm.expect
switch i64 %expval, label %sw.epilog [
i64 1, label %sw.bb
@@ -194,7 +194,7 @@
%tmp = load i32, i32* %x.addr, align 4
%conv = sext i32 %tmp to i64
%expval = call i64 @llvm.expect.i64(i64 %conv, i64 1)
-; CHECK: !prof !3
+; CHECK: !prof !4
; CHECK-NOT: @llvm.expect
switch i64 %expval, label %sw.epilog [
i64 2, label %sw.bb
@@ -278,7 +278,7 @@
%t7 = call i64 @llvm.expect.i64(i64 %t6, i64 0)
%t8 = icmp ne i64 %t7, 0
%t9 = select i1 %t8, i32 1, i32 2
-; CHECK: select{{.*}}, !prof !1
+; CHECK: select{{.*}}, !prof !2
ret i32 %t9
}
@@ -286,6 +286,6 @@
declare i1 @llvm.expect.i1(i1, i1) nounwind readnone
; CHECK: !0 = !{!"branch_weights", i32 2000, i32 1}
-; CHECK: !1 = !{!"branch_weights", i32 1, i32 2000}
-; CHECK: !2 = !{!"branch_weights", i32 1, i32 1, i32 2000}
-; CHECK: !3 = !{!"branch_weights", i32 2000, i32 1, i32 1}
+; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000}
+; CHECK: !3 = !{!"branch_weights", i32 1, i32 1, i32 2000}
+; CHECK: !4 = !{!"branch_weights", i32 2000, i32 1, i32 1}
Index: llvm/lib/Transforms/Utils/MisExpect.cpp
===================================================================
--- llvm/lib/Transforms/Utils/MisExpect.cpp
+++ llvm/lib/Transforms/Utils/MisExpect.cpp
@@ -58,9 +58,10 @@
cl::desc("Use this option to turn on/off "
"warnings about incorrect usage of llvm.expect intrinsics."));
+// Command line option for setting the diagnostic tolerance threshold
static cl::opt<unsigned> MisExpectTolerance(
"misexpect-tolerance", cl::init(0),
- cl::desc("Prevents emiting diagnostics when profile counts are "
+ cl::desc("Prevents emitting diagnostics when profile counts are "
"within N% of the threshold.."));
} // namespace llvm
@@ -90,7 +91,6 @@
// improve diagnostic output, such as the caret. If the same problem exists
// for branch instructions, then we should remove this function and directly
// use the instruction
- //
else if (auto *S = dyn_cast<SwitchInst>(I)) {
Ret = dyn_cast<Instruction>(S->getCondition());
}
@@ -158,15 +158,14 @@
uint64_t TotalBranchWeight =
LikelyBranchWeight + (UnlikelyBranchWeight * NumUnlikelyTargets);
- // FIXME: When we've addressed sample profiling, restore the assertion
- //
- // We cannot calculate branch probability if either of these invariants aren't
- // met. However, MisExpect diagnostics should not prevent code from compiling,
- // so we simply forgo emitting diagnostics here, and return early.
- // assert((TotalBranchWeight >= LikelyBranchWeight) && (TotalBranchWeight > 0)
- // && "TotalBranchWeight is less than the Likely branch weight");
- if ((TotalBranchWeight == 0) || (TotalBranchWeight <= LikelyBranchWeight))
- return;
+ // Failing this assert means that we've either got corrupted metadata, that
+ // we're checking stale or manually inserted branch weights, or that branch
+ // weights are being added multiple times, as is the case for SampleProfiling
+ // under ThinLTO. We gate all known entry paths to verifyMisExpect() by first
+ // checking for the presence of the MD_expected metadata, which is *only*
+ // added in the LowerExpectIntrinsic Pass, avoiding the assert.
+ assert((TotalBranchWeight >= LikelyBranchWeight) && (TotalBranchWeight > 0) &&
+ "TotalBranchWeight is less than the Likely branch weight");
// To determine our threshold value we need to obtain the branch probability
// for the weights added by llvm.expect and use that proportion to calculate
@@ -193,6 +192,14 @@
void checkBackendInstrumentation(Instruction &I,
const ArrayRef<uint32_t> RealWeights) {
+ if (!I.getMetadata(LLVMContext::MD_expected))
+ return;
+ // TODO consider writing a small pass that will remove all MD_expected
+ // metadata that we can run at after the PGO and SampleProfiling passes
+ //
+ // For now, since we're doing the misexpect check here, drop the MD_expected
+ // metadata
+ I.setMetadata(LLVMContext::MD_expected, nullptr);
SmallVector<uint32_t> ExpectedWeights;
if (!extractBranchWeights(I, ExpectedWeights))
return;
Index: llvm/lib/Transforms/Utils/Local.cpp
===================================================================
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -317,6 +317,8 @@
MDBuilder(BB->getContext()).
createBranchWeights(SICase->getValue().getZExtValue(),
SIDef->getValue().getZExtValue()));
+ NewBr->setMetadata(LLVMContext::MD_expected,
+ SI->getMetadata(LLVMContext::MD_expected));
}
// Update make.implicit metadata to the newly-created conditional branch.
@@ -2634,6 +2636,7 @@
case LLVMContext::MD_dbg:
case LLVMContext::MD_tbaa:
case LLVMContext::MD_prof:
+ case LLVMContext::MD_expected:
case LLVMContext::MD_fpmath:
case LLVMContext::MD_tbaa_struct:
case LLVMContext::MD_invariant_load:
Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -107,6 +107,8 @@
SI.setMetadata(LLVMContext::MD_prof,
MDBuilder(CI->getContext()).createBranchWeights(Weights));
+ SI.setMetadata(LLVMContext::MD_expected, MDNode::get(CI->getContext(), None));
+
return true;
}
@@ -255,6 +257,8 @@
BI->setMetadata(LLVMContext::MD_prof,
MDB.createBranchWeights(UnlikelyBranchWeightVal,
LikelyBranchWeightVal));
+ BI->setMetadata(LLVMContext::MD_expected,
+ MDNode::get(BI->getContext(), None));
}
}
@@ -336,6 +340,8 @@
misexpect::checkFrontendInstrumentation(BSI, ExpectedWeights);
BSI.setMetadata(LLVMContext::MD_prof, Node);
+ BSI.setMetadata(LLVMContext::MD_expected,
+ MDNode::get(BSI.getContext(), None));
return true;
}
Index: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
+++ llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
@@ -509,6 +509,7 @@
case LLVMContext::MD_dbg:
case LLVMContext::MD_tbaa:
case LLVMContext::MD_prof:
+ case LLVMContext::MD_expected:
case LLVMContext::MD_fpmath:
case LLVMContext::MD_tbaa_struct:
case LLVMContext::MD_alias_scope:
Index: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -3437,7 +3437,8 @@
NewCall->setAttributes(NewCallerPAL);
// Preserve prof metadata if any.
- NewCall->copyMetadata(*Caller, {LLVMContext::MD_prof});
+ NewCall->copyMetadata(*Caller,
+ {LLVMContext::MD_prof, LLVMContext::MD_expected});
// Insert a cast of the return type as necessary.
Instruction *NC = NewCall;
Index: llvm/lib/Transforms/IPO/LowerTypeTests.cpp
===================================================================
--- llvm/lib/Transforms/IPO/LowerTypeTests.cpp
+++ llvm/lib/Transforms/IPO/LowerTypeTests.cpp
@@ -770,6 +770,8 @@
BranchInst *NewBr = BranchInst::Create(Then, Else, OffsetInRange);
NewBr->setMetadata(LLVMContext::MD_prof,
Br->getMetadata(LLVMContext::MD_prof));
+ NewBr->setMetadata(LLVMContext::MD_expected,
+ Br->getMetadata(LLVMContext::MD_expected));
ReplaceInstWithInst(InitialBB->getTerminator(), NewBr);
// Update phis in Else resulting from InitialBB being split
Index: llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
===================================================================
--- llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
+++ llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
@@ -205,7 +205,8 @@
}
NewCB->setCallingConv(CB->getCallingConv());
NewCB->setAttributes(PAL);
- NewCB->copyMetadata(*CB, {LLVMContext::MD_prof, LLVMContext::MD_dbg});
+ NewCB->copyMetadata(*CB, {LLVMContext::MD_prof, LLVMContext::MD_expected,
+ LLVMContext::MD_dbg});
Args.clear();
@@ -936,7 +937,8 @@
}
NewCB->setCallingConv(CB.getCallingConv());
NewCB->setAttributes(NewCallPAL);
- NewCB->copyMetadata(CB, {LLVMContext::MD_prof, LLVMContext::MD_dbg});
+ NewCB->copyMetadata(CB, {LLVMContext::MD_prof, LLVMContext::MD_expected,
+ LLVMContext::MD_dbg});
Args.clear();
ArgAttrVec.clear();
Index: llvm/lib/Transforms/IPO/Attributor.cpp
===================================================================
--- llvm/lib/Transforms/IPO/Attributor.cpp
+++ llvm/lib/Transforms/IPO/Attributor.cpp
@@ -2675,7 +2675,9 @@
}
// Copy over various properties and the new attributes.
- NewCB->copyMetadata(*OldCB, {LLVMContext::MD_prof, LLVMContext::MD_dbg});
+ NewCB->copyMetadata(*OldCB,
+ {LLVMContext::MD_prof, LLVMContext::MD_expected,
+ LLVMContext::MD_dbg});
NewCB->setCallingConv(OldCB->getCallingConv());
NewCB->takeName(OldCB);
NewCB->setAttributes(AttributeList::get(
Index: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
===================================================================
--- llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
+++ llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
@@ -280,7 +280,8 @@
NewCS->setAttributes(AttributeList::get(F->getContext(),
CallPAL.getFnAttrs(),
CallPAL.getRetAttrs(), ArgAttrVec));
- NewCS->copyMetadata(CB, {LLVMContext::MD_prof, LLVMContext::MD_dbg});
+ NewCS->copyMetadata(CB, {LLVMContext::MD_prof, LLVMContext::MD_expected,
+ LLVMContext::MD_dbg});
Args.clear();
ArgAttrVec.clear();
Index: llvm/include/llvm/IR/FixedMetadataKinds.def
===================================================================
--- llvm/include/llvm/IR/FixedMetadataKinds.def
+++ llvm/include/llvm/IR/FixedMetadataKinds.def
@@ -47,3 +47,4 @@
LLVM_FIXED_MD_KIND(MD_exclude, "exclude", 33)
LLVM_FIXED_MD_KIND(MD_memprof, "memprof", 34)
LLVM_FIXED_MD_KIND(MD_callsite, "callsite", 35)
+LLVM_FIXED_MD_KIND(MD_expected, "expected", 36)
Index: clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp
===================================================================
--- clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp
+++ clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp
@@ -9,7 +9,7 @@
void ab1(int &i) {
// CHECK-LABEL: define{{.*}}ab1
- // CHECK: br {{.*}} !prof [[BW_LIKELY:!.+]]
+ // CHECK: br {{.*}} !prof [[BW_LIKELY:!.+]],
// CHECK: br {{.*}} !prof [[BW_LIKELY]]
// CHECK: br {{.*}} !prof [[BW_LIKELY]]
if (__builtin_expect(a() && b() && a(), 1)) {
@@ -36,7 +36,7 @@
// CHECK: br {{.*}}end{{$}}
// CHECK: br {{.*}}end{{$}}
// CHECK: br {{.*}}end{{$}}
- // CHECK: br {{.*}} !prof [[BW_UNLIKELY:!.+]]
+ // CHECK: br {{.*}} !prof [[BW_UNLIKELY:!.+]],
if (__builtin_expect(a() && b() && c(), 0)) {
++i;
} else {
Index: clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp
===================================================================
--- clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp
+++ clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp
@@ -18,7 +18,7 @@
bool g() {
// CHECK-LABEL: define{{.*}} zeroext i1 @_Z1gv
- // CHECK: br {{.*}} !prof !8
+ // CHECK: br {{.*}} !prof !9
if (b)
[[unlikely]] {
return A();
@@ -29,7 +29,7 @@
bool h() {
// CHECK-LABEL: define{{.*}} zeroext i1 @_Z1hv
- // CHECK: br {{.*}} !prof !8
+ // CHECK: br {{.*}} !prof !9
if (b)
[[unlikely]] return A();
@@ -38,7 +38,7 @@
void NullStmt() {
// CHECK-LABEL: define{{.*}}NullStmt
- // CHECK: br {{.*}} !prof !8
+ // CHECK: br {{.*}} !prof !9
if (b)
[[unlikely]];
else {
@@ -49,7 +49,7 @@
void IfStmt() {
// CHECK-LABEL: define{{.*}}IfStmt
- // CHECK: br {{.*}} !prof !8
+ // CHECK: br {{.*}} !prof !9
if (b)
[[unlikely]] if (B()) {}
@@ -63,7 +63,7 @@
void WhileStmt() {
// CHECK-LABEL: define{{.*}}WhileStmt
- // CHECK: br {{.*}} !prof !8
+ // CHECK: br {{.*}} !prof !9
if (b)
[[unlikely]] while (B()) {}
@@ -76,7 +76,7 @@
void DoStmt() {
// CHECK-LABEL: define{{.*}}DoStmt
- // CHECK: br {{.*}} !prof !8
+ // CHECK: br {{.*}} !prof !9
if (b)
[[unlikely]] do {}
while (B())
@@ -91,7 +91,7 @@
void ForStmt() {
// CHECK-LABEL: define{{.*}}ForStmt
- // CHECK: br {{.*}} !prof !8
+ // CHECK: br {{.*}} !prof !9
if (b)
[[unlikely]] for (; B();) {}
@@ -104,7 +104,7 @@
void GotoStmt() {
// CHECK-LABEL: define{{.*}}GotoStmt
- // CHECK: br {{.*}} !prof !8
+ // CHECK: br {{.*}} !prof !9
if (b)
[[unlikely]] goto end;
else {
@@ -116,7 +116,7 @@
void ReturnStmt() {
// CHECK-LABEL: define{{.*}}ReturnStmt
- // CHECK: br {{.*}} !prof !8
+ // CHECK: br {{.*}} !prof !9
if (b)
[[unlikely]] return;
else {
@@ -127,7 +127,7 @@
void SwitchStmt() {
// CHECK-LABEL: define{{.*}}SwitchStmt
- // CHECK: br {{.*}} !prof !8
+ // CHECK: br {{.*}} !prof !9
if (b)
[[unlikely]] switch (i) {}
else {
@@ -145,4 +145,4 @@
}
// CHECK: !7 = !{!"branch_weights", i32 [[UNLIKELY]], i32 [[LIKELY]]}
-// CHECK: !8 = !{!"branch_weights", i32 [[LIKELY]], i32 [[UNLIKELY]]}
+// CHECK: !9 = !{!"branch_weights", i32 [[LIKELY]], i32 [[UNLIKELY]]}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
