Skip to content

Commit a10802f

Browse files
petrhosekpetrhosek
committedSep 10, 2019
clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM
This patch contains the basic functionality for reporting potentially incorrect usage of __builtin_expect() by comparing the developer's annotation against a collected PGO profile. A more detailed proposal and discussion appears on the CFE-dev mailing list (http://lists.llvm.org/pipermail/cfe-dev/2019-July/062971.html) and a prototype of the initial frontend changes appear here in D65300 We revised the work in D65300 by moving the misexpect check into the LLVM backend, and adding support for IR and sampling based profiles, in addition to frontend instrumentation. We add new misexpect metadata tags to those instructions directly influenced by the llvm.expect intrinsic (branch, switch, and select) when lowering the intrinsics. The misexpect metadata contains information about the expected target of the intrinsic so that we can check against the correct PGO counter when emitting diagnostics, and the compiler's values for the LikelyBranchWeight and UnlikelyBranchWeight. We use these branch weight values to determine when to emit the diagnostic to the user. A future patch should address the comment at the top of LowerExpectIntrisic.cpp to hoist the LikelyBranchWeight and UnlikelyBranchWeight values into a shared space that can be accessed outside of the LowerExpectIntrinsic pass. Once that is done, the misexpect metadata can be updated to be smaller. In the long term, it is possible to reconstruct portions of the misexpect metadata from the existing profile data. However, we have avoided this to keep the code simple, and because some kind of metadata tag will be required to identify which branch/switch/select instructions are influenced by the use of llvm.expect Patch By: paulkirth Differential Revision: https://reviews.llvm.org/D66324 llvm-svn: 371484
·
llvmorg-21-initllvmorg-10.0.0-rc1
1 parent 73da43a commit a10802f

40 files changed

+1704
-26
lines changed
 

‎clang/include/clang/Basic/DiagnosticFrontendKinds.td

+6-1
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,12 @@ def warn_profile_data_missing : Warning<
275275
def warn_profile_data_unprofiled : Warning<
276276
"no profile data available for file \"%0\"">,
277277
InGroup<ProfileInstrUnprofiled>;
278-
278+
def warn_profile_data_misexpect : Warning<
279+
"Potential performance regression from use of __builtin_expect(): "
280+
"Annotation was correct on %0 of profiled executions.">,
281+
BackendInfo,
282+
InGroup<MisExpect>,
283+
DefaultIgnore;
279284
} // end of instrumentation issue category
280285

281286
}

‎clang/include/clang/Basic/DiagnosticGroups.td

+1
Original file line numberDiff line numberDiff line change
@@ -1042,6 +1042,7 @@ def BackendOptimizationFailure : DiagGroup<"pass-failed">;
10421042
def ProfileInstrMissing : DiagGroup<"profile-instr-missing">;
10431043
def ProfileInstrOutOfDate : DiagGroup<"profile-instr-out-of-date">;
10441044
def ProfileInstrUnprofiled : DiagGroup<"profile-instr-unprofiled">;
1045+
def MisExpect : DiagGroup<"misexpect">;
10451046

10461047
// AddressSanitizer frontend instrumentation remarks.
10471048
def SanitizeAddressRemarks : DiagGroup<"sanitize-address">;

‎clang/lib/CodeGen/CodeGenAction.cpp

+26
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include "clang/AST/ASTContext.h"
1515
#include "clang/AST/DeclCXX.h"
1616
#include "clang/AST/DeclGroup.h"
17+
#include "clang/Basic/DiagnosticFrontend.h"
1718
#include "clang/Basic/FileManager.h"
1819
#include "clang/Basic/LangStandard.h"
1920
#include "clang/Basic/SourceManager.h"
@@ -365,6 +366,9 @@ namespace clang {
365366
bool StackSizeDiagHandler(const llvm::DiagnosticInfoStackSize &D);
366367
/// Specialized handler for unsupported backend feature diagnostic.
367368
void UnsupportedDiagHandler(const llvm::DiagnosticInfoUnsupported &D);
369+
/// Specialized handler for misexpect warnings.
370+
/// Note that misexpect remarks are emitted through ORE
371+
void MisExpectDiagHandler(const llvm::DiagnosticInfoMisExpect &D);
368372
/// Specialized handlers for optimization remarks.
369373
/// Note that these handlers only accept remarks and they always handle
370374
/// them.
@@ -617,6 +621,25 @@ void BackendConsumer::UnsupportedDiagHandler(
617621
<< Filename << Line << Column;
618622
}
619623

624+
void BackendConsumer::MisExpectDiagHandler(
625+
const llvm::DiagnosticInfoMisExpect &D) {
626+
StringRef Filename;
627+
unsigned Line, Column;
628+
bool BadDebugInfo = false;
629+
FullSourceLoc Loc =
630+
getBestLocationFromDebugLoc(D, BadDebugInfo, Filename, Line, Column);
631+
632+
Diags.Report(Loc, diag::warn_profile_data_misexpect) << D.getMsg().str();
633+
634+
if (BadDebugInfo)
635+
// If we were not able to translate the file:line:col information
636+
// back to a SourceLocation, at least emit a note stating that
637+
// we could not translate this location. This can happen in the
638+
// case of #line directives.
639+
Diags.Report(Loc, diag::note_fe_backend_invalid_loc)
640+
<< Filename << Line << Column;
641+
}
642+
620643
void BackendConsumer::EmitOptimizationMessage(
621644
const llvm::DiagnosticInfoOptimizationBase &D, unsigned DiagID) {
622645
// We only support warnings and remarks.
@@ -787,6 +810,9 @@ void BackendConsumer::DiagnosticHandlerImpl(const DiagnosticInfo &DI) {
787810
case llvm::DK_Unsupported:
788811
UnsupportedDiagHandler(cast<DiagnosticInfoUnsupported>(DI));
789812
return;
813+
case llvm::DK_MisExpect:
814+
MisExpectDiagHandler(cast<DiagnosticInfoMisExpect>(DI));
815+
return;
790816
default:
791817
// Plugin IDs are not bound to any value as they are set dynamically.
792818
ComputeDiagRemarkID(Severity, backend_plugin, DiagID);

‎clang/lib/Frontend/CompilerInvocation.cpp

+3
Original file line numberDiff line numberDiff line change
@@ -3453,6 +3453,9 @@ bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Res,
34533453
}
34543454
}
34553455

3456+
if (Diags.isIgnored(diag::warn_profile_data_misexpect, SourceLocation()))
3457+
Res.FrontendOpts.LLVMArgs.push_back("-pgo-warn-misexpect");
3458+
34563459
LangOpts.FunctionAlignment =
34573460
getLastArgIntValue(Args, OPT_function_alignment, 0, Diags);
34583461

‎clang/test/Profile/Inputs/misexpect-branch-nonconst-expect-arg.proftext

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
bar
2+
# Func Hash:
3+
11262309464
4+
# Num Counters:
5+
2
6+
# Counter Values:
7+
200000
8+
2
9+

‎clang/test/Profile/Inputs/misexpect-branch.proftext

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
bar
2+
# Func Hash:
3+
45795613684824
4+
# Num Counters:
5+
2
6+
# Counter Values:
7+
200000
8+
0
9+

‎clang/test/Profile/Inputs/misexpect-switch-default-only.proftext

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
main
2+
# Func Hash:
3+
79676873694057560
4+
# Num Counters:
5+
5
6+
# Counter Values:
7+
1
8+
20
9+
20000
10+
20000
11+
20000
12+

‎clang/test/Profile/Inputs/misexpect-switch-default.proftext

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
main
2+
# Func Hash:
3+
8712453512413296413
4+
# Num Counters:
5+
9
6+
# Counter Values:
7+
1
8+
20000
9+
20000
10+
4066
11+
11889
12+
0
13+
0
14+
4045
15+
0
16+

‎clang/test/Profile/Inputs/misexpect-switch.proftext

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
main
2+
# Func Hash:
3+
1965403898329309329
4+
# Num Counters:
5+
9
6+
# Counter Values:
7+
1
8+
20
9+
20000
10+
20000
11+
12
12+
26
13+
0
14+
0
15+
19962
16+

‎clang/test/Profile/misexpect-branch-cold.c

+26
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
// Test that misexpect emits no warning when prediction is correct
2+
3+
// RUN: llvm-profdata merge %S/Inputs/misexpect-branch.proftext -o %t.profdata
4+
// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect
5+
6+
// expected-no-diagnostics
7+
#define likely(x) __builtin_expect(!!(x), 1)
8+
#define unlikely(x) __builtin_expect(!!(x), 0)
9+
10+
int foo(int);
11+
int baz(int);
12+
int buzz();
13+
14+
const int inner_loop = 100;
15+
const int outer_loop = 2000;
16+
17+
int bar() {
18+
int rando = buzz();
19+
int x = 0;
20+
if (unlikely(rando % (outer_loop * inner_loop) == 0)) {
21+
x = baz(rando);
22+
} else {
23+
x = foo(50);
24+
}
25+
return x;
26+
}

‎clang/test/Profile/misexpect-branch-nonconst-expected-val.c

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// Test that misexpect emits no warning when condition is not a compile-time constant
2+
3+
// RUN: llvm-profdata merge %S/Inputs/misexpect-branch-nonconst-expect-arg.proftext -o %t.profdata
4+
// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect
5+
6+
// expected-no-diagnostics
7+
int foo(int);
8+
int baz(int);
9+
int buzz();
10+
11+
const int inner_loop = 100;
12+
const int outer_loop = 2000;
13+
14+
int bar() {
15+
int rando = buzz();
16+
int x = 0;
17+
if (__builtin_expect(rando % (outer_loop * inner_loop) == 0, buzz())) {
18+
x = baz(rando);
19+
} else {
20+
x = foo(50);
21+
}
22+
return x;
23+
}

‎clang/test/Profile/misexpect-branch-unpredictable.c

+25
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// Test that misexpect emits no warning when prediction is correct
2+
3+
// RUN: llvm-profdata merge %S/Inputs/misexpect-branch.proftext -o %t.profdata
4+
// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect
5+
6+
// expected-no-diagnostics
7+
#define unpredictable(x) __builtin_unpredictable(!!(x))
8+
9+
int foo(int);
10+
int baz(int);
11+
int buzz();
12+
13+
const int inner_loop = 100;
14+
const int outer_loop = 2000;
15+
16+
int bar() {
17+
int rando = buzz();
18+
int x = 0;
19+
if (unpredictable(rando % (outer_loop * inner_loop) == 0)) {
20+
x = baz(rando);
21+
} else {
22+
x = foo(50);
23+
}
24+
return x;
25+
}

‎clang/test/Profile/misexpect-branch.c

+28
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// Test that misexpect detects mis-annotated branches
2+
3+
// RUN: llvm-profdata merge %S/Inputs/misexpect-branch.proftext -o %t.profdata
4+
// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify=imprecise -Wmisexpect
5+
// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify=exact -Wmisexpect -debug-info-kind=line-tables-only
6+
// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify=foo
7+
8+
// foo-no-diagnostics
9+
#define likely(x) __builtin_expect(!!(x), 1)
10+
#define unlikely(x) __builtin_expect(!!(x), 0)
11+
12+
int foo(int);
13+
int baz(int);
14+
int buzz();
15+
16+
const int inner_loop = 100;
17+
const int outer_loop = 2000;
18+
19+
int bar() { // imprecise-warning-re {{Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.+}}% ({{[0-9]+ / [0-9]+}}) of profiled executions.}}
20+
int rando = buzz();
21+
int x = 0;
22+
if (likely(rando % (outer_loop * inner_loop) == 0)) { // exact-warning-re {{Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.+}}% ({{[0-9]+ / [0-9]+}}) of profiled executions.}}
23+
x = baz(rando);
24+
} else {
25+
x = foo(50);
26+
}
27+
return x;
28+
}

‎clang/test/Profile/misexpect-switch-default.c

+40
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
// Test that misexpect detects mis-annotated switch statements for default case
2+
3+
// RUN: llvm-profdata merge %S/Inputs/misexpect-switch-default.proftext -o %t.profdata
4+
// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect -debug-info-kind=line-tables-only
5+
6+
int sum(int *buff, int size);
7+
int random_sample(int *buff, int size);
8+
int rand();
9+
void init_arry();
10+
11+
const int inner_loop = 1000;
12+
const int outer_loop = 20;
13+
const int arry_size = 25;
14+
15+
int arry[arry_size] = {0};
16+
17+
int main() {
18+
init_arry();
19+
int val = 0;
20+
int j;
21+
for (j = 0; j < outer_loop * inner_loop; ++j) {
22+
unsigned condition = rand() % 5;
23+
switch (__builtin_expect(condition, 6)) { // expected-warning-re {{Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.+}}% ({{[0-9]+ / [0-9]+}}) of profiled executions.}}
24+
case 0:
25+
val += sum(arry, arry_size);
26+
break;
27+
case 1:
28+
case 2:
29+
case 3:
30+
break;
31+
case 4:
32+
val += random_sample(arry, arry_size);
33+
break;
34+
default:
35+
__builtin_unreachable();
36+
} // end switch
37+
} // end outer_loop
38+
39+
return 0;
40+
}

‎clang/test/Profile/misexpect-switch-nonconst.c

+43
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// Test that misexpect emits no warning when switch condition is non-const
2+
3+
// RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
4+
// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect
5+
6+
// expected-no-diagnostics
7+
int sum(int *buff, int size);
8+
int random_sample(int *buff, int size);
9+
int rand();
10+
void init_arry();
11+
12+
const int inner_loop = 1000;
13+
const int outer_loop = 20;
14+
const int arry_size = 25;
15+
16+
int arry[arry_size] = {0};
17+
18+
int main() {
19+
init_arry();
20+
int val = 0;
21+
22+
int j, k;
23+
for (j = 0; j < outer_loop; ++j) {
24+
for (k = 0; k < inner_loop; ++k) {
25+
unsigned condition = rand() % 10000;
26+
switch (__builtin_expect(condition, rand())) {
27+
case 0:
28+
val += sum(arry, arry_size);
29+
break;
30+
case 1:
31+
case 2:
32+
case 3:
33+
case 4:
34+
val += random_sample(arry, arry_size);
35+
break;
36+
default:
37+
__builtin_unreachable();
38+
} // end switch
39+
} // end inner_loop
40+
} // end outer_loop
41+
42+
return 0;
43+
}

‎clang/test/Profile/misexpect-switch-only-default-case.c

+35
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// Test that misexpect emits no warning when there is only one switch case
2+
3+
// RUN: llvm-profdata merge %S/Inputs/misexpect-switch-default-only.proftext -o %t.profdata
4+
// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect -debug-info-kind=line-tables-only
5+
6+
// expected-no-diagnostics
7+
int sum(int *buff, int size);
8+
int random_sample(int *buff, int size);
9+
int rand();
10+
void init_arry();
11+
12+
const int inner_loop = 1000;
13+
const int outer_loop = 20;
14+
const int arry_size = 25;
15+
16+
int arry[arry_size] = {0};
17+
18+
int main() {
19+
init_arry();
20+
int val = 0;
21+
22+
int j, k;
23+
for (j = 0; j < outer_loop; ++j) {
24+
for (k = 0; k < inner_loop; ++k) {
25+
unsigned condition = rand() % 10000;
26+
switch (__builtin_expect(condition, 0)) {
27+
default:
28+
val += random_sample(arry, arry_size);
29+
break;
30+
}; // end switch
31+
} // end inner_loop
32+
} // end outer_loop
33+
34+
return 0;
35+
}

0 commit comments

Comments
 (0)
Please sign in to comment.