Skip to content

[InstrProf] Support conditional counter updates #102542

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 16, 2024
Merged

[InstrProf] Support conditional counter updates #102542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f915b0c
[InstrProf] Support conditional counter updates
gulfemsavrun Aug 8, 2024
d195d63
Update llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
gulfemsavrun Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@ cl::opt<bool> AtomicFirstCounter(
"the entry counter)"),
cl::init(false));

cl::opt<bool> ConditionalCounterUpdate(
"conditional-counter-update",
cl::desc("Do conditional counter updates in single byte counters mode)"),
cl::init(false));

// If the option is not specified, the default behavior about whether
// counter promotion is done depends on how instrumentaiton lowering
// pipeline is setup, i.e., the default value of true of this option
Expand Down Expand Up @@ -1213,6 +1218,17 @@ Value *InstrLowerer::getBitmapAddress(InstrProfMCDCTVBitmapUpdate *I) {
void InstrLowerer::lowerCover(InstrProfCoverInst *CoverInstruction) {
auto *Addr = getCounterAddress(CoverInstruction);
IRBuilder<> Builder(CoverInstruction);
if (ConditionalCounterUpdate) {
Instruction *SplitBefore = CoverInstruction->getNextNode();
auto &Ctx = CoverInstruction->getParent()->getContext();
auto *Int8Ty = llvm::Type::getInt8Ty(Ctx);
Value *Load = Builder.CreateLoad(Int8Ty, Addr, "pgocount");
Value *Cmp = Builder.CreateIsNotNull(Load, "pgocount.ifnonzero");
Instruction *ThenBranch =
SplitBlockAndInsertIfThen(Cmp, SplitBefore, false);
Comment on lines +1227 to +1228
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ellishg Can we create an additional block here? I remember the past discussion. #95585 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok now because of a recent change. See my comment #102542 (comment)
But thanks for bringing this up!

Copy link
Contributor

@ellishg ellishg Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading my old comment again, I remembered that it's important that we compute the CFG hash before changing the CFG, otherwise IRPGO will not correctly attribute profile counts. Luckily we compute the CFG hash in the earlier pass PGOInstrumentationGen.

llvm-project/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

Lines 919 to 923 in c66dee4

// llvm.instrprof.cover(i8* <name>, i64 <hash>, i32 <num-counters>,
// i32 <index>)
Builder.CreateCall(
Intrinsic::getDeclaration(&M, Intrinsic::instrprof_cover),
{NormalizedNamePtr, CFGHash, Builder.getInt32(1), Builder.getInt32(0)});

My apologies, my original comment in #95585 (comment) was wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, it is ok to add a block here, right? If that's the case, I'm going to resolve this conversation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll follow bitmap part in #110792.

Builder.SetInsertPoint(ThenBranch);
}

// We store zero to represent that this block is covered.
Builder.CreateStore(Builder.getInt8(0), Addr);
CoverInstruction->eraseFromParent();
Expand Down
69 changes: 69 additions & 0 deletions llvm/test/Instrumentation/InstrProfiling/conditional-counter-updates.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
; RUN: opt < %s -S -passes=instrprof -conditional-counter-update | FileCheck %s

target triple = "x86_64-unknown-linux-gnu"

@__profn_foo = private constant [3 x i8] c"foo"
@__profn_bar = private constant [3 x i8] c"bar"

; CHECK-LABEL: define void @foo
; CHECK-NEXT: %pgocount = load i8, ptr @__profc_foo, align 1
; CHECK-NEXT: %pgocount.ifnonzero = icmp ne i8 %pgocount, 0
; CHECK-NEXT: br i1 %pgocount.ifnonzero, label %1, label %2

; CHECK-LABEL: 1:
; CHECK-NEXT: store i8 0, ptr @__profc_foo, align 1
; CHECK-NEXT: br label %2

; CHECK-LABEL: 2:
; CHECK-NEXT: ret void
define void @foo() {
call void @llvm.instrprof.cover(ptr @__profn_foo, i64 0, i32 1, i32 0)
ret void
}

; CHECK-LABEL: define i32 @bar
; CHECK-LABEL: entry:
; CHECK-NEXT: %retval = alloca i32, align 4
; CHECK-NEXT: %cond.addr = alloca i32, align 4
; CHECK-NEXT: store i32 %cond, ptr %cond.addr, align 4
; CHECK-NEXT: %pgocount = load i8, ptr @__profc_bar, align 1
; CHECK-NEXT: %pgocount.ifnonzero = icmp ne i8 %pgocount, 0
; CHECK-NEXT: br i1 %pgocount.ifnonzero, label %0, label %1

; CHECK-LABEL: 0: ; preds = %entry
; CHECK-NEXT: store i8 0, ptr @__profc_bar, align 1
; CHECK-NEXT: br label %1

; CHECK-LABEL: 1: ; preds = %entry, %0
; CHECK-NEXT: %2 = load i32, ptr %cond.addr, align 4
; CHECK-NEXT: %cmp = icmp slt i32 %2, 0
; CHECK-NEXT: br i1 %cmp, label %if.then, label %if.end

; CHECK-LABEL: if.then: ; preds = %1
; CHECK-NEXT: %pgocount1 = load i8, ptr getelementptr inbounds ([3 x i8], ptr @__profc_bar, i32 0, i32 1), align 1
; CHECK-NEXT: %pgocount.ifnonzero2 = icmp ne i8 %pgocount1, 0
; CHECK-NEXT: br i1 %pgocount.ifnonzero2, label %3, label %4
define i32 @bar(i32 %cond) #0 {
entry:
%retval = alloca i32, align 4
%cond.addr = alloca i32, align 4
store i32 %cond, ptr %cond.addr, align 4
call void @llvm.instrprof.cover(ptr @__profn_bar, i64 0, i32 3, i32 0)
%0 = load i32, ptr %cond.addr, align 4
%cmp = icmp slt i32 %0, 0
br i1 %cmp, label %if.then, label %if.end

if.then: ; preds = %entry
call void @llvm.instrprof.cover(ptr @__profn_bar, i64 0, i32 3, i32 1)
store i32 -1, ptr %retval, align 4
br label %return

if.end: ; preds = %entry
call void @llvm.instrprof.cover(ptr @__profn_bar, i64 0, i32 3, i32 2)
store i32 0, ptr %retval, align 4
br label %return

return: ; preds = %if.end, %if.then
%1 = load i32, ptr %retval, align 4
ret i32 %1
}
Loading