Cleanup MC/DC intrinsics for #82448 #95496
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3rd arg of `tvbitmap.update` was made unused. Remove 3rd arg. Sweep `condbitmap.update`, since it is no longer used.
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-llvm-transforms Author: NAKAMURA Takumi (chapuni) Changes3rd arg of Sweep Full diff: https://github.com/llvm/llvm-project/pull/95496.diff 9 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 59139e342de88..2839697614595 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -1260,9 +1260,8 @@ void CodeGenPGO::emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder,
// from a pointer to a dedicated temporary value on the stack that is itself
// updated via emitMCDCCondBitmapReset() and emitMCDCCondBitmapUpdate(). The
// index represents an executed test vector.
- llvm::Value *Args[5] = {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
+ llvm::Value *Args[4] = {llvm::ConstantExpr::getBitCast(FuncNameVar, I8PtrTy),
Builder.getInt64(FunctionHash),
- Builder.getInt32(0), // Unused
Builder.getInt32(MCDCTestVectorBitmapOffset),
MCDCCondBitmapAddr.emitRawPointer(CGF)};
Builder.CreateCall(
diff --git a/clang/test/Profile/c-mcdc.c b/clang/test/Profile/c-mcdc.c
index 251c18baa861d..7c1d734028364 100644
--- a/clang/test/Profile/c-mcdc.c
+++ b/clang/test/Profile/c-mcdc.c
@@ -82,7 +82,7 @@ int test(int a, int b, int c, int d, int e, int f) {
// UPDATE FINAL BITMASK WITH RESULT.
// NOPROFPASS-LABEL: lor.end:
-// NOPROFPASS: call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @__profn_test, i64 [[HASH]], i32 0, i32 0, ptr %mcdc.addr)
+// NOPROFPASS: call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @__profn_test, i64 [[HASH]], i32 0, ptr %mcdc.addr)
// MCDC-DAG: %[[TEMP0:mcdc.temp[0-9]*]] = load i32, ptr %mcdc.addr, align 4
// MCDC: %[[TEMP:[0-9]+]] = add i32 %[[TEMP0]], 0
// MCDC: %[[LAB1:[0-9]+]] = lshr i32 %[[TEMP]], 3
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index 10d53bea149ef..a587afed6ca88 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -14441,52 +14441,6 @@ to generate the appropriate data structures and the code to instrument MC/DC
test vectors in a format that can be written out by a compiler runtime and
consumed via the ``llvm-profdata`` tool.
-'``llvm.instrprof.mcdc.condbitmap.update``' Intrinsic
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-Syntax:
-"""""""
-
-::
-
- declare void @llvm.instrprof.mcdc.condbitmap.update(ptr <name>, i64 <hash>,
- i32 <condition-id>,
- ptr <mcdc-temp-addr>,
- i1 <bool-value>)
-
-Overview:
-"""""""""
-
-The '``llvm.instrprof.mcdc.condbitmap.update``' intrinsic is used to track
-MC/DC condition evaluation for each condition in a boolean expression.
-
-Arguments:
-""""""""""
-
-The first argument is a pointer to a global variable containing the
-name of the entity being instrumented. This should generally be the
-(mangled) function name for a set of counters.
-
-The second argument is a hash value that can be used by the consumer
-of the profile data to detect changes to the instrumented source.
-
-The third argument is an ID of a condition to track. This value is used as a
-bit index into the condition bitmap.
-
-The fourth argument is the address of the condition bitmap.
-
-The fifth argument is the boolean value representing the evaluation of the
-condition (true or false)
-
-Semantics:
-""""""""""
-
-This intrinsic represents the update of a condition bitmap that is local to a
-function and will cause the ``-instrprof`` pass to generate the code to
-instrument the control flow around each condition in a boolean expression. The
-ID of each condition corresponds to a bit index in the condition bitmap which
-is set based on the evaluation of the condition.
-
'``llvm.instrprof.mcdc.tvbitmap.update``' Intrinsic
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -14496,7 +14450,6 @@ Syntax:
::
declare void @llvm.instrprof.mcdc.tvbitmap.update(ptr <name>, i64 <hash>,
- i32 <unused>)
i32 <bitmap-index>,
ptr <mcdc-temp-addr>)
@@ -14520,12 +14473,10 @@ name of the entity being instrumented. This should generally be the
The second argument is a hash value that can be used by the consumer
of the profile data to detect changes to the instrumented source.
-The third argument is not used.
-
-The fourth argument is the bit index into the global test vector bitmap
+The third argument is the bit index into the global test vector bitmap
corresponding to the function.
-The fifth argument is the address of the condition bitmap, which contains a
+The fourth argument is the address of the condition bitmap, which contains a
value representing an executed MC/DC test vector. It is loaded and used as the
bit index of the test vector bitmap.
diff --git a/llvm/include/llvm/IR/IntrinsicInst.h b/llvm/include/llvm/IR/IntrinsicInst.h
index 1ac4a5fffb43b..3963a5c8ab8f9 100644
--- a/llvm/include/llvm/IR/IntrinsicInst.h
+++ b/llvm/include/llvm/IR/IntrinsicInst.h
@@ -1455,9 +1455,7 @@ class InstrProfInstBase : public IntrinsicInst {
public:
static bool classof(const Value *V) {
if (const auto *Instr = dyn_cast<IntrinsicInst>(V))
- return isCounterBase(*Instr) || isMCDCBitmapBase(*Instr) ||
- Instr->getIntrinsicID() ==
- Intrinsic::instrprof_mcdc_condbitmap_update;
+ return isCounterBase(*Instr) || isMCDCBitmapBase(*Instr);
return false;
}
// The name of the instrumented function.
@@ -1618,43 +1616,14 @@ class InstrProfMCDCTVBitmapUpdate : public InstrProfMCDCBitmapInstBase {
/// \return The index of the TestVector Bitmap upon which this intrinsic
/// acts.
ConstantInt *getBitmapIndex() const {
- return cast<ConstantInt>(const_cast<Value *>(getArgOperand(3)));
+ return cast<ConstantInt>(const_cast<Value *>(getArgOperand(2)));
}
/// \return The address of the corresponding condition bitmap containing
/// the index of the TestVector to update within the TestVector Bitmap.
- Value *getMCDCCondBitmapAddr() const {
- return cast<Value>(const_cast<Value *>(getArgOperand(4)));
- }
-};
-
-/// This represents the llvm.instrprof.mcdc.condbitmap.update intrinsic.
-/// It does not pertain to global bitmap updates or parameters and so doesn't
-/// inherit from InstrProfMCDCBitmapInstBase.
-class InstrProfMCDCCondBitmapUpdate : public InstrProfInstBase {
-public:
- static bool classof(const IntrinsicInst *I) {
- return I->getIntrinsicID() == Intrinsic::instrprof_mcdc_condbitmap_update;
- }
- static bool classof(const Value *V) {
- return isa<IntrinsicInst>(V) && classof(cast<IntrinsicInst>(V));
- }
-
- /// \return The ID of the condition to update.
- ConstantInt *getCondID() const {
- return cast<ConstantInt>(const_cast<Value *>(getArgOperand(2)));
- }
-
- /// \return The address of the corresponding condition bitmap.
Value *getMCDCCondBitmapAddr() const {
return cast<Value>(const_cast<Value *>(getArgOperand(3)));
}
-
- /// \return The boolean value to set in the condition bitmap for the
- /// corresponding condition ID. This represents how the condition evaluated.
- Value *getCondBool() const {
- return cast<Value>(const_cast<Value *>(getArgOperand(4)));
- }
};
class PseudoProbeInst : public IntrinsicInst {
diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td
index 4c506a6ace23e..ef500329d1fb9 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -939,12 +939,7 @@ def int_instrprof_mcdc_parameters : Intrinsic<[],
// A test vector bitmap update for instrumentation based MCDC profiling.
def int_instrprof_mcdc_tvbitmap_update : Intrinsic<[],
[llvm_ptr_ty, llvm_i64_ty,
- llvm_i32_ty, llvm_i32_ty, llvm_ptr_ty]>;
-
-// A condition bitmap value update for instrumentation based MCDC profiling.
-def int_instrprof_mcdc_condbitmap_update : Intrinsic<[],
- [llvm_ptr_ty, llvm_i64_ty,
- llvm_i32_ty, llvm_ptr_ty, llvm_i1_ty]>;
+ llvm_i32_ty, llvm_ptr_ty]>;
def int_call_preallocated_setup : DefaultAttrsIntrinsic<[llvm_token_ty], [llvm_i32_ty]>;
def int_call_preallocated_arg : DefaultAttrsIntrinsic<[llvm_ptr_ty], [llvm_token_ty, llvm_i32_ty]>;
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index be5e0f6ef058b..d5e298f13a9fd 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -7565,8 +7565,6 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
llvm_unreachable("instrprof failed to lower mcdc parameters");
case Intrinsic::instrprof_mcdc_tvbitmap_update:
llvm_unreachable("instrprof failed to lower an mcdc tvbitmap update");
- case Intrinsic::instrprof_mcdc_condbitmap_update:
- llvm_unreachable("instrprof failed to lower an mcdc condbitmap update");
case Intrinsic::localescape: {
MachineFunction &MF = DAG.getMachineFunction();
const TargetInstrInfo *TII = DAG.getSubtarget().getInstrInfo();
diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
index 0c79eaa812b5f..9c34374f0ece1 100644
--- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -278,10 +278,6 @@ class InstrLowerer final {
/// using the index represented by the a temp value into a bitmap.
void lowerMCDCTestVectorBitmapUpdate(InstrProfMCDCTVBitmapUpdate *Ins);
- /// Replace instrprof.mcdc.temp.update with a shift and or instruction using
- /// the corresponding condition ID.
- void lowerMCDCCondBitmapUpdate(InstrProfMCDCCondBitmapUpdate *Ins);
-
/// Compute the address of the counter value that this profiling instruction
/// acts on.
Value *getCounterAddress(InstrProfCntrInstBase *I);
@@ -648,9 +644,6 @@ bool InstrLowerer::lowerIntrinsics(Function *F) {
} else if (auto *IPBU = dyn_cast<InstrProfMCDCTVBitmapUpdate>(&Instr)) {
lowerMCDCTestVectorBitmapUpdate(IPBU);
MadeChange = true;
- } else if (auto *IPTU = dyn_cast<InstrProfMCDCCondBitmapUpdate>(&Instr)) {
- lowerMCDCCondBitmapUpdate(IPTU);
- MadeChange = true;
}
}
}
@@ -1053,34 +1046,6 @@ void InstrLowerer::lowerMCDCTestVectorBitmapUpdate(
Update->eraseFromParent();
}
-void InstrLowerer::lowerMCDCCondBitmapUpdate(
- InstrProfMCDCCondBitmapUpdate *Update) {
- IRBuilder<> Builder(Update);
- auto *Int32Ty = Type::getInt32Ty(M.getContext());
- auto *MCDCCondBitmapAddr = Update->getMCDCCondBitmapAddr();
-
- // Load the MCDC temporary value from the stack.
- // %mcdc.temp = load i32, ptr %mcdc.addr, align 4
- auto *Temp = Builder.CreateLoad(Int32Ty, MCDCCondBitmapAddr, "mcdc.temp");
-
- // Zero-extend the evaluated condition boolean value (0 or 1) by 32bits.
- // %1 = zext i1 %tobool to i32
- auto *CondV_32 = Builder.CreateZExt(Update->getCondBool(), Int32Ty);
-
- // Shift the boolean value left (by the condition's ID) to form a bitmap.
- // %2 = shl i32 %1, <Update->getCondID()>
- auto *ShiftedVal = Builder.CreateShl(CondV_32, Update->getCondID());
-
- // Perform logical OR of the bitmap against the loaded MCDC temporary value.
- // %3 = or i32 %mcdc.temp, %2
- auto *Result = Builder.CreateOr(Temp, ShiftedVal);
-
- // Store the updated temporary value back to the stack.
- // store i32 %3, ptr %mcdc.addr, align 4
- Builder.CreateStore(Result, MCDCCondBitmapAddr);
- Update->eraseFromParent();
-}
-
/// Get the name of a profiling variable for a particular function.
static std::string getVarName(InstrProfInstBase *Inc, StringRef Prefix,
bool &Renamed) {
diff --git a/llvm/test/Instrumentation/InstrProfiling/mcdc.ll b/llvm/test/Instrumentation/InstrProfiling/mcdc.ll
index e9ae80891ea6e..4980b45f90c50 100644
--- a/llvm/test/Instrumentation/InstrProfiling/mcdc.ll
+++ b/llvm/test/Instrumentation/InstrProfiling/mcdc.ll
@@ -22,14 +22,7 @@ entry:
%0 = load i32, ptr %A.addr, align 4
%tobool = icmp ne i32 %0, 0
- call void @llvm.instrprof.mcdc.condbitmap.update(ptr @__profn_test, i64 99278, i32 0, ptr %mcdc.addr, i1 %tobool)
- ; CHECK: %[[TEMP:mcdc.*]] = load i32, ptr %mcdc.addr, align 4
- ; CHECK-NEXT: %[[LAB1:[0-9]+]] = zext i1 %tobool to i32
- ; CHECK-NEXT: %[[LAB2:[0-9]+]] = shl i32 %[[LAB1]], 0
- ; CHECK-NEXT: %[[LAB3:[0-9]+]] = or i32 %[[TEMP]], %[[LAB2]]
- ; CHECK-NEXT: store i32 %[[LAB3]], ptr %mcdc.addr, align 4
-
- call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @__profn_test, i64 99278, i32 1, i32 0, ptr %mcdc.addr)
+ call void @llvm.instrprof.mcdc.tvbitmap.update(ptr @__profn_test, i64 99278, i32 0, ptr %mcdc.addr)
; CHECK: %[[TEMP0:mcdc.*]] = load i32, ptr %mcdc.addr, align 4
; CHECK-NEXT: %[[TEMP:[0-9]+]] = add i32 %[[TEMP0]], 0
; CHECK-NEXT: %[[LAB4:[0-9]+]] = lshr i32 %[[TEMP]], 3
@@ -47,6 +40,4 @@ declare void @llvm.instrprof.cover(ptr, i64, i32, i32)
declare void @llvm.instrprof.mcdc.parameters(ptr, i64, i32)
-declare void @llvm.instrprof.mcdc.condbitmap.update(ptr, i64, i32, ptr, i1)
-
-declare void @llvm.instrprof.mcdc.tvbitmap.update(ptr, i64, i32, i32, ptr)
+declare void @llvm.instrprof.mcdc.tvbitmap.update(ptr, i64, i32, ptr)
diff --git a/llvm/unittests/IR/IntrinsicsTest.cpp b/llvm/unittests/IR/IntrinsicsTest.cpp
index dddd2f73d4446..6f9e724c40326 100644
--- a/llvm/unittests/IR/IntrinsicsTest.cpp
+++ b/llvm/unittests/IR/IntrinsicsTest.cpp
@@ -77,7 +77,6 @@ TEST_F(IntrinsicsTest, InstrProfInheritance) {
return isa<TYPE>(I) && is##PARENT(I); \
}
__ISA(InstrProfCntrInstBase, InstrProfInstBase);
- __ISA(InstrProfMCDCCondBitmapUpdate, InstrProfInstBase);
__ISA(InstrProfCoverInst, InstrProfCntrInstBase);
__ISA(InstrProfIncrementInst, InstrProfCntrInstBase);
__ISA(InstrProfIncrementInstStep, InstrProfIncrementInst);
@@ -96,8 +95,6 @@ TEST_F(IntrinsicsTest, InstrProfInheritance) {
{Intrinsic::instrprof_increment, isInstrProfIncrementInst},
{Intrinsic::instrprof_increment_step, isInstrProfIncrementInstStep},
{Intrinsic::instrprof_callsite, isInstrProfCallsite},
- {Intrinsic::instrprof_mcdc_condbitmap_update,
- isInstrProfMCDCCondBitmapUpdate},
{Intrinsic::instrprof_mcdc_parameters,
isInstrProfMCDCBitmapParameters},
{Intrinsic::instrprof_mcdc_tvbitmap_update,
|
It's used by Rust: https://github.com/rust-lang/rust/blob/fd7eefc2753e867053a1c567a7b504ae308e3f85/compiler/rustc_llvm/llvm-wrapper/RustWrapper.cpp#L1581 What's the migration path? Could you put something in the release notes for other users hitting this? |
|
@zmodem Thanks for the notification. Re. condbitmap.update, it has become useless after #82448 . I think pruning it will be the possible way. |
Can you expand on this? I'm not familiar with these intrinsics at all. What are users who were using |
|
@zmodem #82448 doesn't use intermediate condbitmap but integer index. I decided not to modify and use the intrinsic In contrast, I modified I guess they would hit apparently buggy behavior if they were using obsolete Since I didn't update and won't update the semantics of |
|
I've created #95887 (Release notes). |
chapuni
added a commit
that referenced
this pull request
Jun 18, 2024
AlexisPerry
pushed a commit
to llvm-project-tlp/llvm-project
that referenced
this pull request
Jul 9, 2024
Mostly apparent changes (llvm#82448, llvm#95496) are described here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
3rd arg of
tvbitmap.updatewas made unused. Remove 3rd arg.Sweep
condbitmap.update, since it is no longer used.