Skip to content
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

[MC/DC][Coverage] Split out Read-modfy-Write to rmw_or(ptr,i8) #96040

Merged
merged 4 commits into from
Jun 22, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
62 changes: 51 additions & 11 deletions llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,10 @@ class InstrLowerer final {
GlobalVariable *NamesVar = nullptr;
size_t NamesSize = 0;

/// The instance of [[forceinline]] rmw_or(ptr, i8).
/// This is name-insensitive.
Function *RMWOrFunc = nullptr;

// vector of counter load/store pairs to be register promoted.
std::vector<LoadStorePair> PromotionCandidates;

Expand Down Expand Up @@ -297,6 +301,14 @@ class InstrLowerer final {
StringRef Name,
GlobalValue::LinkageTypes Linkage);

/// Create [[forceinline]] rmw_or(ptr, i8).
/// This doesn't update `RMWOrFunc`.
Function *createRMWOrFunc();

/// Get the call to `rmw_or`.
/// Create the instance if it is unknown.
CallInst *getRMWOrCall(Value *Addr, Value *Val);

/// Compute the address of the test vector bitmap that this profiling
/// instruction acts on.
Value *getBitmapAddress(InstrProfMCDCTVBitmapUpdate *I);
Expand Down Expand Up @@ -937,6 +949,44 @@ Value *InstrLowerer::getCounterAddress(InstrProfCntrInstBase *I) {
return Builder.CreateIntToPtr(Add, Addr->getType());
}

Function *InstrLowerer::createRMWOrFunc() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you document this function with basic C code to make is easy to understand what it's doing.

auto &Ctx = M.getContext();
auto *Int8Ty = Type::getInt8Ty(Ctx);
// void alwaysinline rmw_or(ptr, i8)
Function *Fn = Function::Create(
FunctionType::get(Type::getVoidTy(Ctx),
{PointerType::getUnqual(Ctx), Int8Ty}, false),
Function::LinkageTypes::PrivateLinkage, "rmw_or", M);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a convention for naming compiler-generated functions? Will there be problems if a user creates a function with the same name? Or is this ok becase the linkage is private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose private named functions wouldn't clash. Let me reconfirm later.
(I guess it might be unnamed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reconfirmed this won't clash. If I called createRMWOrFunc() twice, I got both @rmw_or and @rmw_or.1.

This may work with unnamed.

Fn->addFnAttr(Attribute::AlwaysInline);
auto *ArgAddr = Fn->getArg(0);
auto *ArgVal = Fn->getArg(1);
IRBuilder<> Builder(BasicBlock::Create(Ctx, "", Fn));

// Load profile bitmap byte.
// %mcdc.bits = load i8, ptr %4, align 1
auto *Bitmap = Builder.CreateLoad(Int8Ty, ArgAddr, "mcdc.bits");

// Perform logical OR of profile bitmap byte and shifted bit offset.
// %8 = or i8 %mcdc.bits, %7
auto *Result = Builder.CreateOr(Bitmap, ArgVal);

// Store the updated profile bitmap byte.
// store i8 %8, ptr %3, align 1
Builder.CreateStore(Result, ArgAddr);

// Terminator
Builder.CreateRetVoid();

return Fn;
}

CallInst *InstrLowerer::getRMWOrCall(Value *Addr, Value *Val) {
if (!RMWOrFunc)
RMWOrFunc = createRMWOrFunc();

return CallInst::Create(RMWOrFunc, {Addr, Val});
}

Value *InstrLowerer::getBitmapAddress(InstrProfMCDCTVBitmapUpdate *I) {
auto *Bitmaps = getOrCreateRegionBitmaps(I);
IRBuilder<> Builder(I);
Expand Down Expand Up @@ -1044,17 +1094,7 @@ void InstrLowerer::lowerMCDCTestVectorBitmapUpdate(
// %7 = shl i8 1, %6
auto *ShiftedVal = Builder.CreateShl(Builder.getInt8(0x1), BitToSet);

// Load profile bitmap byte.
// %mcdc.bits = load i8, ptr %4, align 1
auto *Bitmap = Builder.CreateLoad(Int8Ty, BitmapByteAddr, "mcdc.bits");

// Perform logical OR of profile bitmap byte and shifted bit offset.
// %8 = or i8 %mcdc.bits, %7
auto *Result = Builder.CreateOr(Bitmap, ShiftedVal);

// Store the updated profile bitmap byte.
// store i8 %8, ptr %3, align 1
Builder.CreateStore(Result, BitmapByteAddr);
Builder.Insert(getRMWOrCall(BitmapByteAddr, ShiftedVal));
Update->eraseFromParent();
}

Expand Down
10 changes: 7 additions & 3 deletions llvm/test/Instrumentation/InstrProfiling/mcdc.ll
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,16 @@ entry:
; CHECK-NEXT: %[[LAB8:[0-9]+]] = and i32 %[[TEMP]], 7
; CHECK-NEXT: %[[LAB9:[0-9]+]] = trunc i32 %[[LAB8]] to i8
; CHECK-NEXT: %[[LAB10:[0-9]+]] = shl i8 1, %[[LAB9]]
; CHECK-NEXT: %[[BITS:mcdc.*]] = load i8, ptr %[[LAB7]], align 1
; BASIC-NEXT: %[[LAB11:[0-9]+]] = or i8 %[[BITS]], %[[LAB10]]
; BASIC-NEXT: store i8 %[[LAB11]], ptr %[[LAB7]], align 1
; CHECK-NEXT: call void @[[RMW_OR:.+]](ptr %[[LAB7]], i8 %[[LAB10]])
ret void
}

; CHECK: define private void @[[RMW_OR]](ptr %[[ARGPTR:.+]], i8 %[[ARGVAL:.+]])
; CHECK: %[[BITS:.+]] = load i8, ptr %[[ARGPTR]], align 1
; BASIC-NEXT: %[[LAB11:[0-9]+]] = or i8 %[[BITS]], %[[ARGVAL]]
; BASIC-NEXT: store i8 %[[LAB11]], ptr %[[ARGPTR]], align 1
; CHECK-NEXT: ret void

declare void @llvm.instrprof.cover(ptr, i64, i32, i32)

declare void @llvm.instrprof.mcdc.parameters(ptr, i64, i32)
Expand Down
Loading