[PATCH] D39933: Summary: Fix out-of-order stepping behavior in programs with sunk instructions

[Voss, Matthew via llvm-commits]

That makes sense. I’ll add the test and resubmit.

From: David Blaikie [mailto:dblaikie at gmail.com]
Sent: Monday, November 20, 2017 1:16 PM
To: reviews+D39933+public+5dbf07e50e16ff55 at reviews.llvm.org
Cc: Voss, Matthew; aprantl at apple.com; Robinson, Paul; echristo at gmail.com; llvm-commits at lists.llvm.org
Subject: Re: [PATCH] D39933: Summary: Fix out-of-order stepping behavior in programs with sunk instructions

Should this code have more test coverage? Looks like it's only looking for the absence of a location. Should there be testing for the merge case as well?
On Fri, Nov 10, 2017 at 5:11 PM Matthew Voss via Phabricator <reviews at reviews.llvm.org<mailto:reviews at reviews.llvm.org>> wrote:
ormris created this revision.

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.


https://reviews.llvm.org/D39933

Files:
  lib/CodeGen/MachineSink.cpp
  test/CodeGen/X86/machinesink-debuginfo.ll


Index: test/CodeGen/X86/machinesink-debuginfo.ll
===================================================================
--- /dev/null
+++ test/CodeGen/X86/machinesink-debuginfo.ll
@@ -0,0 +1,49 @@
+; RUN: llc -O2 -o - < %s | 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: .loc 1 2 26
+  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<http://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)
Index: lib/CodeGen/MachineSink.cpp
===================================================================
--- lib/CodeGen/MachineSink.cpp
+++ lib/CodeGen/MachineSink.cpp
@@ -36,6 +36,7 @@
 #include "llvm/CodeGen/TargetInstrInfo.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"
@@ -868,6 +869,16 @@
   SmallVector<MachineInstr *, 2> DbgValuesToSink;
   collectDebugValues(MI, DbgValuesToSink);

+  // Merge or erase debug location to ensure consistent stepping in profilers
+  // and debuggers.
+  if (!SuccToSinkTo->empty()) {
+    MI.setDebugLoc(DILocation::getMergedLocation(MI.getDebugLoc(),
+                                                 InsertPos->getDebugLoc()));
+  } else {
+    MI.setDebugLoc(DebugLoc());
+  }
+
+
   // Move the instruction.
   SuccToSinkTo->splice(InsertPos, ParentBlock, MI,
                        ++MachineBasicBlock::iterator(MI));

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171120/26bd1d33/attachment.html>