[llvm] [speculative-execution] Hoists debug values unnecessarily. (PR #85782)
Carlos Alberto Enciso via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 1 21:54:14 PDT 2024
https://github.com/CarlosAlbertoEnciso updated https://github.com/llvm/llvm-project/pull/85782
>From 60621d1528fb721097c5f229059b7f41dc601515 Mon Sep 17 00:00:00 2001
From: Carlos Alberto Enciso <carlos.alberto.enciso at gmail.com>
Date: Tue, 19 Mar 2024 11:58:03 +0000
Subject: [PATCH 1/2] [speculative-execution] Hoists debug values
unnecessarily.
After https://reviews.llvm.org/D81730:
`SpeculativeExecutionPass::considerHoistingFromTo` hoists
instructions, including debug intrinsics, as long as none
of their used values are instructions that appear prior
in the block that are not being hoisted.
This behaviour has been duplicated for DPValues to get
rid of a binary difference.
The correct solution is not hoist these debug values at
all, whichever format they're in.
---
.../Scalar/SpeculativeExecution.cpp | 50 +++++++++----------
.../SpeculativeExecution/PR46267.ll | 2 +-
2 files changed, 25 insertions(+), 27 deletions(-)
diff --git a/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp b/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
index 44ea6e9ffefa2b..7ac3cf6357e7c4 100644
--- a/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
+++ b/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
@@ -260,10 +260,31 @@ static InstructionCost ComputeSpeculationCost(const Instruction *I,
}
}
+// Do not hoist any debug info intrinsics.
+// ...
+// if (cond) {
+// x = y * z;
+// foo();
+// }
+// ...
+// -------- Which then becomes:
+// ...
+// if.then:
+// %x = mul i32 %y, %z
+// call void @llvm.dbg.value(%x, !"x", !DIExpression())
+// call void foo()
+//
+// If SpeculativeExecution might decide to hoist the 'y * z' calculation
+// out of the 'if' block, because it is more efficient that way, so the
+// '%x = mul i32 %y, %z' moves to the block above. But it might also
+// decide to hoist the 'llvm.dbg.value' call.
+// This is incorrect, because even if we've moved the calculation of
+// 'y * z', we should not see the value of 'x' change unless we
+// actually go inside the 'if' block.
+
bool SpeculativeExecutionPass::considerHoistingFromTo(
BasicBlock &FromBlock, BasicBlock &ToBlock) {
SmallPtrSet<const Instruction *, 8> NotHoisted;
- SmallDenseMap<const Instruction *, SmallVector<DPValue *>> DPValuesToHoist;
auto HasNoUnhoistedInstr = [&NotHoisted](auto Values) {
for (const Value *V : Values) {
if (const auto *I = dyn_cast_or_null<Instruction>(V))
@@ -274,15 +295,8 @@ bool SpeculativeExecutionPass::considerHoistingFromTo(
};
auto AllPrecedingUsesFromBlockHoisted =
[&HasNoUnhoistedInstr](const User *U) {
- // Debug variable has special operand to check it's not hoisted.
- if (const auto *DVI = dyn_cast<DbgVariableIntrinsic>(U))
- return HasNoUnhoistedInstr(DVI->location_ops());
-
- // Usially debug label intrinsic corresponds to label in LLVM IR. In
- // these cases we should not move it here.
- // TODO: Possible special processing needed to detect it is related to a
- // hoisted instruction.
- if (isa<DbgLabelInst>(U))
+ // Do not hoist any debug info intrinsics.
+ if (isa<DbgInfoIntrinsic>(U))
return false;
return HasNoUnhoistedInstr(U->operand_values());
@@ -291,12 +305,6 @@ bool SpeculativeExecutionPass::considerHoistingFromTo(
InstructionCost TotalSpeculationCost = 0;
unsigned NotHoistedInstCount = 0;
for (const auto &I : FromBlock) {
- // Make note of any DPValues that need hoisting. DPLabels
- // get left behind just like llvm.dbg.labels.
- for (DPValue &DPV : filterDbgVars(I.getDbgRecordRange())) {
- if (HasNoUnhoistedInstr(DPV.location_ops()))
- DPValuesToHoist[DPV.getInstruction()].push_back(&DPV);
- }
const InstructionCost Cost = ComputeSpeculationCost(&I, *TTI);
if (Cost.isValid() && isSafeToSpeculativelyExecute(&I) &&
AllPrecedingUsesFromBlockHoisted(&I)) {
@@ -314,16 +322,6 @@ bool SpeculativeExecutionPass::considerHoistingFromTo(
}
for (auto I = FromBlock.begin(); I != FromBlock.end();) {
- // If any DPValues attached to this instruction should be hoisted, hoist
- // them now - they will end up attached to either the next hoisted
- // instruction or the ToBlock terminator.
- if (DPValuesToHoist.contains(&*I)) {
- for (auto *DPV : DPValuesToHoist[&*I]) {
- DPV->removeFromParent();
- ToBlock.insertDbgRecordBefore(DPV,
- ToBlock.getTerminator()->getIterator());
- }
- }
// We have to increment I before moving Current as moving Current
// changes the list that I is iterating through.
auto Current = I;
diff --git a/llvm/test/Transforms/SpeculativeExecution/PR46267.ll b/llvm/test/Transforms/SpeculativeExecution/PR46267.ll
index d940ee6a7863d7..69dac2220d9a64 100644
--- a/llvm/test/Transforms/SpeculativeExecution/PR46267.ll
+++ b/llvm/test/Transforms/SpeculativeExecution/PR46267.ll
@@ -31,7 +31,6 @@ define void @f(i32 %i) {
entry:
; CHECK-LABEL: @f(
; CHECK: %a2 = add i32 %i, 0
-; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 %a2
br i1 undef, label %land.rhs, label %land.end
land.rhs: ; preds = %entry
@@ -42,6 +41,7 @@ land.rhs: ; preds = %entry
; CHECK-NEXT: %a0 = load i32, ptr undef, align 1
; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 %a0
; CHECK-NEXT: call void @llvm.dbg.label
+; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 %a2
call void @llvm.dbg.label(metadata !11), !dbg !10
%y = alloca i32, align 4
call void @llvm.dbg.declare(metadata ptr %y, metadata !14, metadata !DIExpression()), !dbg !10
>From f2c2cdf57477d95d8c9e49c5363f84848df4b335 Mon Sep 17 00:00:00 2001
From: Carlos Alberto Enciso
<47597242+CarlosAlbertoEnciso at users.noreply.github.com>
Date: Tue, 2 Apr 2024 05:40:24 +0100
Subject: [PATCH 2/2] [speculative-execution] Hoists debug values
unnecessarily.
After https://reviews.llvm.org/D81730:
`SpeculativeExecutionPass::considerHoistingFromTo` hoists
instructions, including debug intrinsics, as long as none
of their used values are instructions that appear prior
in the block that are not being hoisted.
This behaviour has been duplicated for DPValues to get
rid of a binary difference.
The correct solution is not hoist these debug values at
all, whichever format they're in.
nit: Remove 'If' from comment.
---
llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp b/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
index 7ac3cf6357e7c4..f921ee72a0a1ce 100644
--- a/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
+++ b/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
@@ -274,7 +274,7 @@ static InstructionCost ComputeSpeculationCost(const Instruction *I,
// call void @llvm.dbg.value(%x, !"x", !DIExpression())
// call void foo()
//
-// If SpeculativeExecution might decide to hoist the 'y * z' calculation
+// SpeculativeExecution might decide to hoist the 'y * z' calculation
// out of the 'if' block, because it is more efficient that way, so the
// '%x = mul i32 %y, %z' moves to the block above. But it might also
// decide to hoist the 'llvm.dbg.value' call.
More information about the llvm-commits
mailing list