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

[chapuni]

rmw_or is defined as "private alwaysinline". At the moment, it has just only simple "Read, Or, and Write", which is just same as the current implementation.

I expect this doesn't change actual behavior in runtime.

[llvmbot]

@llvm/pr-subscribers-pgo

@llvm/pr-subscribers-llvm-transforms

Author: NAKAMURA Takumi (chapuni)

Changes

rmw_or is defined as "private alwaysinline". At the moment, it has just only simple "Read, Or, and Write", which is just same as the current implementation.

I expect this doesn't change actual behavior in runtime.

---

Full diff: https://github.com/llvm/llvm-project/pull/96040.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp (+51-11)
• (modified) llvm/test/Instrumentation/InstrProfiling/mcdc.ll (+8-4)

diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
index b9f1fcdd9c233..e8dce21b2361d 100644
--- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -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;
 
@@ -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);
@@ -937,6 +949,44 @@ Value *InstrLowerer::getCounterAddress(InstrProfCntrInstBase *I) {
   return Builder.CreateIntToPtr(Add, Addr->getType());
 }
 
+Function *InstrLowerer::createRMWOrFunc() {
+  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);
+  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);
@@ -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();
 }
 
diff --git a/llvm/test/Instrumentation/InstrProfiling/mcdc.ll b/llvm/test/Instrumentation/InstrProfiling/mcdc.ll
index 4980b45f90c50..35abb1071c92c 100644
--- a/llvm/test/Instrumentation/InstrProfiling/mcdc.ll
+++ b/llvm/test/Instrumentation/InstrProfiling/mcdc.ll
@@ -1,5 +1,5 @@
 ; Check that MC/DC intrinsics are properly lowered
-; RUN: opt < %s -passes=instrprof -S | FileCheck %s
+; RUN: opt < %s -passes=instrprof -S | FileCheck %s --check-prefixes=CHECK,BASIC
 ; RUN: opt < %s -passes=instrprof -runtime-counter-relocation -S 2>&1 | FileCheck %s --check-prefix RELOC
 
 ; RELOC: Runtime counter relocation is presently not supported for MC/DC bitmaps
@@ -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
-  ; CHECK-NEXT: %[[LAB11:[0-9]+]] = or i8 %[[BITS]], %[[LAB10]]
-  ; CHECK-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)

[chapuni]

@evodius96 Thanks!

@ellishg WDYT?

[ellishg]

Is MC/DC expected to ever be used without a profile runtime library? If not, you could create a new function in compiler-rt instead of creating it in IR.

[evodius96]

Is MC/DC expected to ever be used without a profile runtime library? If not, you could create a new function in compiler-rt instead of creating it in IR.

Yes, effectively -- at least our downstream embedded toolchain for baremetal targets uses a stripped down compiler-rt.

[ellishg]

Seems ok to me, thanks!

[ellishg]

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

[ellishg]

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?

[chapuni]

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

[chapuni]

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.