-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[profcheck] Add unknown branch weights for inlined strcmp/strncmp #160455
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
[profcheck] Add unknown branch weights for inlined strcmp/strncmp #160455
Conversation
@llvm/pr-subscribers-llvm-transforms Author: Jin Huang (jinhuang1102) ChangesThe strcmp/strncmp inliner creates new conditional branches but was failing to add profile metadata. This caused the ProfileVerifierPass to fail when profcheck is enabled. This patch fixes the issue by explicitly adding unknown branch weights to these branches. Full diff: https://github.com/llvm/llvm-project/pull/160455.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp b/llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp
index 40de36d81ddd2..d00a3bc86d6b8 100644
--- a/llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp
+++ b/llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp
@@ -29,6 +29,7 @@
#include "llvm/IR/Function.h"
#include "llvm/IR/IRBuilder.h"
#include "llvm/IR/PatternMatch.h"
+#include "llvm/IR/ProfDataUtils.h"
#include "llvm/Transforms/Utils/BasicBlockUtils.h"
#include "llvm/Transforms/Utils/BuildLibCalls.h"
#include "llvm/Transforms/Utils/Local.h"
@@ -1283,11 +1284,14 @@ void StrNCmpInliner::inlineCompare(Value *LHS, StringRef RHS, uint64_t N,
Value *VR =
ConstantInt::get(CI->getType(), static_cast<unsigned char>(RHS[i]));
Value *Sub = Swapped ? B.CreateSub(VR, VL) : B.CreateSub(VL, VR);
- if (i < N - 1)
- B.CreateCondBr(B.CreateICmpNE(Sub, ConstantInt::get(CI->getType(), 0)),
- BBNE, BBSubs[i + 1]);
- else
+ if (i < N - 1) {
+ BranchInst *CondBrInst = B.CreateCondBr(
+ B.CreateICmpNE(Sub, ConstantInt::get(CI->getType(), 0)), BBNE,
+ BBSubs[i + 1]);
+ setExplicitlyUnknownBranchWeights(*CondBrInst, DEBUG_TYPE);
+ } else {
B.CreateBr(BBNE);
+ }
Phi->addIncoming(Sub, BBSubs[i]);
}
|
BranchInst *CondBrInst = B.CreateCondBr( | ||
B.CreateICmpNE(Sub, ConstantInt::get(CI->getType(), 0)), BBNE, | ||
BBSubs[i + 1]); | ||
setExplicitlyUnknownBranchWeights(*CondBrInst, DEBUG_TYPE); |
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.
Set this only if the function entry count is non-zero. See this discussion: #158743 (comment)
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 also need a unittest. You could modify or clone the one that failed originally, to also have a function entry count and then you can check the unknown
insertion.
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 looks good with a unit test and the entry count change.
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.
Added the entry_count checking and modified the IR test case for !prof check.
(took the liberty and added the reference to the tracking issue) |
7795f82
to
7848841
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
25af10f
to
6912751
Compare
@@ -53,4 +53,6 @@ declare i32 @strcmp(ptr, ptr) | |||
; CHECK: [[DBG4]] = !DILocation(line: 258, column: 10, scope: [[META5:![0-9]+]]) | |||
; CHECK: [[META5]] = distinct !DISubprogram(name: "streq", scope: [[META1]], file: [[META1]], line: 257, type: [[META6:![0-9]+]], scopeLine: 257, spFlags: DISPFlagDefinition, unit: [[META0]], retainedNodes: [[META2]]) | |||
; CHECK: [[META6]] = !DISubroutineType(types: [[META2]]) | |||
; CHECK: [[PROF8]] = !{!"function_entry_count", i64 1000} |
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.
@mtrofin Add function entry count and check for unknown insert here.
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 you need to give main
here its function_entry_count
!prof
explicitly, otherwise this only passes in the build with profcheck enabled.
otherwise lgtm
224bbc7
to
48f2644
Compare
@@ -5,22 +5,23 @@ | |||
|
|||
@.str = constant [3 x i8] c"-h\00" | |||
|
|||
define i32 @main() { | |||
; CHECK-LABEL: define i32 @main() { | |||
define i32 @main() !prof !8{ |
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.
nit: place a space after !8
404d2f9
to
f737e69
Compare
…Transforms/AggressiveInstCombine
f737e69
to
23308e8
Compare
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/23166 Here is the relevant piece of the build log for the reference
|
…vm#160455) The strcmp/strncmp inliner creates new conditional branches but was failing to add profile metadata. This caused the ProfileVerifierPass to fail when profcheck is enabled. This patch fixes the issue by explicitly adding unknown branch weights to these branches. Issue llvm#147390
The strcmp/strncmp inliner creates new conditional branches but was failing to add profile metadata. This caused the ProfileVerifierPass to fail when profcheck is enabled.
This patch fixes the issue by explicitly adding unknown branch weights to these branches.
Issue #147390