Summary: Fix out-of-order stepping behavior in programs with sunk instructions

[ormris]

Summary

MachineSink attempts to place instructions near the basic blocks where
they are needed.

Once an instruction has been sunk, its location relative to other
instructions no longer is consistent with the original source code. In
order to ensure correct stepping in the debugger, the debug location for
sunk instructions is either merged with the insertion point or erased if the
target successor block is empty.

[ormris]

Friendly ping.

[probinson]

LGTM. I'll commit this for you, as you don't have commit access yet.

[echristo]

llvm/trunk/lib/CodeGen/MachineSink.cpp

874 ↗

(On Diff #123621)

We don't put braces around single statement if conditions. Should probably also update the comments (in a follow-up comment) for the debug value moving to be clear what's going on there versus the location.

[ormris]

llvm/trunk/lib/CodeGen/MachineSink.cpp

874 ↗

(On Diff #123621)

Will do.

[ormris]

Change list:

• Ensure that we don't crash when the insert position is not found.
• Add comment explaining the debug value move.
• Add test that tests the merging of debug values.
• Update the previous test to use MIR.

[aprantl]

lib/CodeGen/MachineSink.cpp

885

adjacent

[ormris]

Fix comment spelling.

[ormris]

Friendly ping.

[probinson]

LGTM again.

[ormris]

Thanks, @probinson! If someone could commit this, that would be great. I don't have commit access currently.

lib/CodeGen/MachineSink.cpp

 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/MachinePostDominators.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/BranchProbability.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
bool MachineSinking::SinkInstruction(MachineInstr &MI, bool &SawStore,
   MachineBasicBlock::iterator InsertPos = SuccToSinkTo->begin();
   while (InsertPos != SuccToSinkTo->end() && InsertPos->isPHI())
     ++InsertPos;
 
   // collect matching debug values.
   SmallVector<MachineInstr *, 2> DbgValuesToSink;
   collectDebugValues(MI, DbgValuesToSink);
 
+  // Merge or erase debug location to ensure consistent stepping in profilers
+  // and debuggers.
+  if (!SuccToSinkTo->empty() && InsertPos != SuccToSinkTo->end())
+    MI.setDebugLoc(DILocation::getMergedLocation(MI.getDebugLoc(),
+                                                 InsertPos->getDebugLoc()));
+  else
+    MI.setDebugLoc(DebugLoc());
+
+
   // Move the instruction.
   SuccToSinkTo->splice(InsertPos, ParentBlock, MI,
                        ++MachineBasicBlock::iterator(MI));
 
-  // Move debug values.
+  // Move previously adjecent debug value instructions to the insert position.
   for (SmallVectorImpl<MachineInstr *>::iterator DBI = DbgValuesToSink.begin(),
          DBE = DbgValuesToSink.end(); DBI != DBE; ++DBI) {
     MachineInstr *DbgMI = *DBI;
     SuccToSinkTo->splice(InsertPos, ParentBlock,  DbgMI,
                          ++MachineBasicBlock::iterator(DbgMI));
   }
 
   // Conservatively, clear any kill flags, since it's possible that they are no

test/CodeGen/X86/machinesink-merge-debuginfo.ll

+; RUN: llc -simplify-mir -stop-after=machine-sink < %s -o - | FileCheck %s
+
+; ModuleID = 'test-sink-debug.cpp'
+source_filename = "test-sink-debug.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind readnone uwtable
+define double @_Z3fooddb(double %x, double %y, i1 zeroext %c) local_unnamed_addr !dbg !7 {
+  tail call void @llvm.dbg.value(metadata double %x, metadata !13, metadata !DIExpression()), !dbg !16
+  tail call void @llvm.dbg.value(metadata double %y, metadata !14, metadata !DIExpression()), !dbg !16
+  tail call void @llvm.dbg.value(metadata i1 %c, metadata !15, metadata !DIExpression()), !dbg !16
+  %a = fdiv double %x, 3.000000e+00
+  %b = fdiv double %y, 5.000000e+00, !dbg !17
+  br i1 %c, label %first, label %second
+first:
+  %e = fadd double %a, 1.000000e+00
+  br label %final
+second:
+  %f = fadd double %b, 1.000000e+00, !dbg !17
+; CHECK:  debug-location !17
+; CHECK:  debug-location !17
+  br label %final
+final:
+  %cond = phi double [%e, %first], [%f, %second]
+  %d = fadd double %cond, 1.000000e+00
+  ret double %d
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+attributes #1 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 6.0.0 (trunk 313291)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "test-sink-debug.cpp", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 6.0.0 (trunk 313291)"}
+!7 = distinct !DISubprogram(name: "foo", linkageName: "_Z3fooddb", scope: !1, file: !1, line: 1, type: !8, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !12)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !10, !10, !11}
+!10 = !DIBasicType(name: "double", size: 64, encoding: DW_ATE_float)
+!11 = !DIBasicType(name: "bool", size: 8, encoding: DW_ATE_boolean)
+!12 = !{!13, !14, !15}
+!13 = !DILocalVariable(name: "x", arg: 1, scope: !7, file: !1, line: 1, type: !10)
+!14 = !DILocalVariable(name: "y", arg: 2, scope: !7, file: !1, line: 1, type: !10)
+!15 = !DILocalVariable(name: "c", arg: 3, scope: !7, file: !1, line: 1, type: !11)
+!16 = !DILocation(line: 1, column: 19, scope: !7)
+!17 = !DILocation(line: 2, column: 26, scope: !7)

test/CodeGen/X86/machinesink-null-debuginfo.ll

+; RUN: llc -simplify-mir -stop-after=machine-sink < %s -o - | FileCheck %s
+
+; ModuleID = 'test-sink-debug.cpp'
+source_filename = "test-sink-debug.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nounwind readnone uwtable
+define double @_Z3fooddb(double %x, double %y, i1 zeroext %c) local_unnamed_addr !dbg !7 {
+  tail call void @llvm.dbg.value(metadata double %x, metadata !13, metadata !DIExpression()), !dbg !16
+  tail call void @llvm.dbg.value(metadata double %y, metadata !14, metadata !DIExpression()), !dbg !17
+  tail call void @llvm.dbg.value(metadata i1 %c, metadata !15, metadata !DIExpression()), !dbg !18
+  %a = fdiv double %x, 3.000000e+00
+  %b = fdiv double %y, 5.000000e+00, !dbg !21
+  %cond = select i1 %c,  double %a, double %b
+; CHECK-NOT: debug-location !21
+  ret double %cond, !dbg !22
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+attributes #1 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 6.0.0 (trunk 313291)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "test-sink-debug.cpp", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 6.0.0 (trunk 313291)"}
+!7 = distinct !DISubprogram(name: "foo", linkageName: "_Z3fooddb", scope: !1, file: !1, line: 1, type: !8, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !12)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !10, !10, !11}
+!10 = !DIBasicType(name: "double", size: 64, encoding: DW_ATE_float)
+!11 = !DIBasicType(name: "bool", size: 8, encoding: DW_ATE_boolean)
+!12 = !{!13, !14, !15}
+!13 = !DILocalVariable(name: "x", arg: 1, scope: !7, file: !1, line: 1, type: !10)
+!14 = !DILocalVariable(name: "y", arg: 2, scope: !7, file: !1, line: 1, type: !10)
+!15 = !DILocalVariable(name: "c", arg: 3, scope: !7, file: !1, line: 1, type: !11)
+!16 = !DILocation(line: 1, column: 19, scope: !7)
+!17 = !DILocation(line: 1, column: 29, scope: !7)
+!18 = !DILocation(line: 1, column: 37, scope: !7)
+!21 = !DILocation(line: 2, column: 26, scope: !7)
+!22 = !DILocation(line: 2, column: 3, scope: !7)