[speculative-execution] Hoists debug values unnecessarily.

llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp

@@ -260,11 +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()
+//
+// 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<DbgVariableRecord *>>
-      DbgVariableRecordsToHoist;
   auto HasNoUnhoistedInstr = [&NotHoisted](auto Values) {
     for (const Value *V : Values) {
       if (const auto *I = dyn_cast_or_null<Instruction>(V))
@@ -275,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());
@@ -292,12 +305,6 @@ bool SpeculativeExecutionPass::considerHoistingFromTo(
   InstructionCost TotalSpeculationCost = 0;
   unsigned NotHoistedInstCount = 0;
   for (const auto &I : FromBlock) {
-    // Make note of any DbgVariableRecords that need hoisting. DbgLabelRecords
-    // get left behind just like llvm.dbg.labels.
-    for (DbgVariableRecord &DVR : filterDbgVars(I.getDbgRecordRange())) {
-      if (HasNoUnhoistedInstr(DVR.location_ops()))
-        DbgVariableRecordsToHoist[DVR.getInstruction()].push_back(&DVR);
-    }
     const InstructionCost Cost = ComputeSpeculationCost(&I, *TTI);
     if (Cost.isValid() && isSafeToSpeculativelyExecute(&I) &&
         AllPrecedingUsesFromBlockHoisted(&I)) {
@@ -315,16 +322,6 @@ bool SpeculativeExecutionPass::considerHoistingFromTo(
   }
 
   for (auto I = FromBlock.begin(); I != FromBlock.end();) {
-    // If any DbgVariableRecords 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 (DbgVariableRecordsToHoist.contains(&*I)) {
-      for (auto *DVR : DbgVariableRecordsToHoist[&*I]) {
-        DVR->removeFromParent();
-        ToBlock.insertDbgRecordBefore(DVR,
-                                      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;

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