Skip to content

Commit 60621d1

Browse files
[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.
1 parent c1ccf07 commit 60621d1

File tree

2 files changed

+25
-27
lines changed

2 files changed

+25
-27
lines changed

llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp

Lines changed: 24 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -260,10 +260,31 @@ static InstructionCost ComputeSpeculationCost(const Instruction *I,
260260
}
261261
}
262262

263+
// Do not hoist any debug info intrinsics.
264+
// ...
265+
// if (cond) {
266+
// x = y * z;
267+
// foo();
268+
// }
269+
// ...
270+
// -------- Which then becomes:
271+
// ...
272+
// if.then:
273+
// %x = mul i32 %y, %z
274+
// call void @llvm.dbg.value(%x, !"x", !DIExpression())
275+
// call void foo()
276+
//
277+
// If SpeculativeExecution might decide to hoist the 'y * z' calculation
278+
// out of the 'if' block, because it is more efficient that way, so the
279+
// '%x = mul i32 %y, %z' moves to the block above. But it might also
280+
// decide to hoist the 'llvm.dbg.value' call.
281+
// This is incorrect, because even if we've moved the calculation of
282+
// 'y * z', we should not see the value of 'x' change unless we
283+
// actually go inside the 'if' block.
284+
263285
bool SpeculativeExecutionPass::considerHoistingFromTo(
264286
BasicBlock &FromBlock, BasicBlock &ToBlock) {
265287
SmallPtrSet<const Instruction *, 8> NotHoisted;
266-
SmallDenseMap<const Instruction *, SmallVector<DPValue *>> DPValuesToHoist;
267288
auto HasNoUnhoistedInstr = [&NotHoisted](auto Values) {
268289
for (const Value *V : Values) {
269290
if (const auto *I = dyn_cast_or_null<Instruction>(V))
@@ -274,15 +295,8 @@ bool SpeculativeExecutionPass::considerHoistingFromTo(
274295
};
275296
auto AllPrecedingUsesFromBlockHoisted =
276297
[&HasNoUnhoistedInstr](const User *U) {
277-
// Debug variable has special operand to check it's not hoisted.
278-
if (const auto *DVI = dyn_cast<DbgVariableIntrinsic>(U))
279-
return HasNoUnhoistedInstr(DVI->location_ops());
280-
281-
// Usially debug label intrinsic corresponds to label in LLVM IR. In
282-
// these cases we should not move it here.
283-
// TODO: Possible special processing needed to detect it is related to a
284-
// hoisted instruction.
285-
if (isa<DbgLabelInst>(U))
298+
// Do not hoist any debug info intrinsics.
299+
if (isa<DbgInfoIntrinsic>(U))
286300
return false;
287301

288302
return HasNoUnhoistedInstr(U->operand_values());
@@ -291,12 +305,6 @@ bool SpeculativeExecutionPass::considerHoistingFromTo(
291305
InstructionCost TotalSpeculationCost = 0;
292306
unsigned NotHoistedInstCount = 0;
293307
for (const auto &I : FromBlock) {
294-
// Make note of any DPValues that need hoisting. DPLabels
295-
// get left behind just like llvm.dbg.labels.
296-
for (DPValue &DPV : filterDbgVars(I.getDbgRecordRange())) {
297-
if (HasNoUnhoistedInstr(DPV.location_ops()))
298-
DPValuesToHoist[DPV.getInstruction()].push_back(&DPV);
299-
}
300308
const InstructionCost Cost = ComputeSpeculationCost(&I, *TTI);
301309
if (Cost.isValid() && isSafeToSpeculativelyExecute(&I) &&
302310
AllPrecedingUsesFromBlockHoisted(&I)) {
@@ -314,16 +322,6 @@ bool SpeculativeExecutionPass::considerHoistingFromTo(
314322
}
315323

316324
for (auto I = FromBlock.begin(); I != FromBlock.end();) {
317-
// If any DPValues attached to this instruction should be hoisted, hoist
318-
// them now - they will end up attached to either the next hoisted
319-
// instruction or the ToBlock terminator.
320-
if (DPValuesToHoist.contains(&*I)) {
321-
for (auto *DPV : DPValuesToHoist[&*I]) {
322-
DPV->removeFromParent();
323-
ToBlock.insertDbgRecordBefore(DPV,
324-
ToBlock.getTerminator()->getIterator());
325-
}
326-
}
327325
// We have to increment I before moving Current as moving Current
328326
// changes the list that I is iterating through.
329327
auto Current = I;

llvm/test/Transforms/SpeculativeExecution/PR46267.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ define void @f(i32 %i) {
3131
entry:
3232
; CHECK-LABEL: @f(
3333
; CHECK: %a2 = add i32 %i, 0
34-
; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 %a2
3534
br i1 undef, label %land.rhs, label %land.end
3635

3736
land.rhs: ; preds = %entry
@@ -42,6 +41,7 @@ land.rhs: ; preds = %entry
4241
; CHECK-NEXT: %a0 = load i32, ptr undef, align 1
4342
; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 %a0
4443
; CHECK-NEXT: call void @llvm.dbg.label
44+
; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 %a2
4545
call void @llvm.dbg.label(metadata !11), !dbg !10
4646
%y = alloca i32, align 4
4747
call void @llvm.dbg.declare(metadata ptr %y, metadata !14, metadata !DIExpression()), !dbg !10

0 commit comments

Comments
 (0)