SpeculativeExecution: fix incorrect debug info move

[dfukalov]

Summary

Debug info related instructions got zero cost so hoisted unconditionally

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=46267

[aprantl]

llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp

259

Please always use full sentences with a . at the end in comments.

268

Could you please add a comment as to why it shouldn't be moved? That will probably be more helpful to the reader.

269

processing

[dfukalov]

updated comments

[dfukalov]

ping...

[aprantl]

I'm not super confident that this won't have any unforeseen consequences, but the results in the testcase look right to me.

llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Scalar/SpeculativeExecution.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/Analysis/GlobalsModRef.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/Instructions.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 
 using namespace llvm;
 
switch (Operator::getOpcode(I)) {
     default:
       return UINT_MAX; // Disallow anything not explicitly listed.
   }
 }
 
 bool SpeculativeExecutionPass::considerHoistingFromTo(
     BasicBlock &FromBlock, BasicBlock &ToBlock) {
   SmallPtrSet<const Instruction *, 8> NotHoisted;
-  const auto AllPrecedingUsesFromBlockHoisted = [&NotHoisted](User *U) {
-    for (Value* V : U->operand_values()) {
-      if (Instruction *I = dyn_cast<Instruction>(V)) {
+  const auto AllPrecedingUsesFromBlockHoisted = [&NotHoisted](const User *U) {
+    // Debug variable has special operand to check it's not hoisted.
+    if (const auto *DVI = dyn_cast<DbgVariableIntrinsic>(U)) {
+      if (const auto *I =
+              dyn_cast_or_null<Instruction>(DVI->getVariableLocation()))
+        if (NotHoisted.count(I) == 0)
+          return true;
+      return false;
+    }
+
+    // Usially debug label instrinsic 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))
+      return false;
+
+    for (const Value *V : U->operand_values()) {
+      if (const Instruction *I = dyn_cast<Instruction>(V)) {
         if (NotHoisted.count(I) > 0)
           return false;
       }
     }
     return true;
   };
 
   unsigned TotalSpeculationCost = 0;
-  for (auto& I : FromBlock) {
+  unsigned NotHoistedInstCount = 0;
+  for (const auto &I : FromBlock) {
     const unsigned Cost = ComputeSpeculationCost(&I, *TTI);
     if (Cost != UINT_MAX && isSafeToSpeculativelyExecute(&I) &&
         AllPrecedingUsesFromBlockHoisted(&I)) {
       TotalSpeculationCost += Cost;
       if (TotalSpeculationCost > SpecExecMaxSpeculationCost)
         return false;  // too much to hoist
     } else {
-      NotHoisted.insert(&I);
-      if (NotHoisted.size() > SpecExecMaxNotHoisted)
+      // If the instruction cannot be hoisted but has zero cost suppose it's
+      // a special case e.g. debug info instrinsics that should not be counted
+      // for threshold.
+      if (Cost)
+        NotHoistedInstCount++;
+      if (NotHoistedInstCount > SpecExecMaxNotHoisted)
         return false; // too much left behind
+      NotHoisted.insert(&I);
     }
   }
 
   for (auto I = FromBlock.begin(); I != FromBlock.end();) {
     // We have to increment I before moving Current as moving Current
     // changes the list that I is iterating through.
     auto Current = I;
     ++I;

llvm/test/Transforms/SpeculativeExecution/PR46267.ll

+; RUN: opt < %s -S -speculative-execution | FileCheck %s
+; RUN: opt < %s -S -passes='speculative-execution' | FileCheck %s
+
+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
+; CHECK: land.rhs:
+; CHECK-NEXT: call void @llvm.dbg.label
+; CHECK-NEXT: %x = alloca i32, align 4
+; CHECK-NEXT: call void @llvm.dbg.addr(metadata i32* %x
+; CHECK-NEXT: %y = alloca i32, align 4
+; CHECK-NEXT: call void @llvm.dbg.declare(metadata i32* %y
+; CHECK-NEXT: %a0 = load i32, i32* undef, align 1
+; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 %a0
+  call void @llvm.dbg.label(metadata !11), !dbg !10
+  %x = alloca i32, align 4
+  call void @llvm.dbg.addr(metadata i32* %x, metadata !12, metadata !DIExpression()), !dbg !10
+  %y = alloca i32, align 4
+  call void @llvm.dbg.declare(metadata i32* %y, metadata !14, metadata !DIExpression()), !dbg !10
+
+  %a0 = load i32, i32* undef, align 1
+  call void @llvm.dbg.value(metadata i32 %a0, metadata !9, metadata !DIExpression()), !dbg !10
+
+  %a2 = add i32 %i, 0
+  call void @llvm.dbg.value(metadata i32 %a2, metadata !13, metadata !DIExpression()), !dbg !10
+
+  br label %land.end
+
+land.end:                                         ; preds = %land.rhs, %entry
+  ret void
+}
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+declare void @llvm.dbg.label(metadata)
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+declare void @llvm.dbg.addr(metadata, metadata, metadata)
+
+attributes #1 = { nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "foo.c", directory: "/bar")
+!2 = !{}
+!3 = !{!4}
+!4 = !DIBasicType(name: "int", size: 16, encoding: DW_ATE_signed)
+!5 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 1, type: !7, scopeLine: 2, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!7 = !DISubroutineType(types: !8)
+!8 = !{null}
+!9 = !DILocalVariable(name: "a0", scope: !6, file: !1, line: 3, type: !4)
+!10 = !DILocation(line: 0, scope: !6)
+!11 = !DILabel(scope: !6, name: "label", file: !1, line: 1)
+!12 = !DILocalVariable(name: "x", scope: !6, file: !1, line: 3, type: !4)
+!13 = !DILocalVariable(name: "a2", scope: !6, file: !1, line: 3, type: !4)
+!14 = !DILocalVariable(name: "y", scope: !6, file: !1, line: 3, type: !4)