Add DBG_VALUE support to the linear DAG scheduler

[jcranmer]

Summary

I noticed that the fast/linear DAG scheduler doesn't lower DBG_VALUEs except for function entry nodes. This copies what appears to be the relevant code, and it does seem to work for most of the simple testing I've done.

[bogner]

Seems reasonable. Adding a couple of folks who know a bit more about debug info than I do to approve this.

[vsk]

Thanks for working on this.

lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp

791 ↗

(On Diff #133242)

Nit: could you used a range-for loop here and run clang-format over the diff?

test/CodeGen/Generic/linear-dbg-value.ll

29 ↗

(On Diff #133242)

I'm not able to follow what's changed based on these check lines. Could you attach llc's pre-patch/post-patch output, so we can see how the output changes?

[probinson]

lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp

797 ↗

(On Diff #133242)

Don't put braces around a single-line body.
Could compact these 4 lines to 2:

if (auto *DbgMI = Emitter.EmitDbgValue(DVs[i], VRBaseMap))
  BB->insert(InsertPos, DbgMI);

[jcranmer]

Before patch:

pre-patch.out7 KBDownload

After patch:

post-patch.out7 KBDownload

Effectively, before this patch, the only dbg.value entries that get added are those that have to do with function arguments. The ones in the middle of the function are completely ignored.

[aprantl]

test/CodeGen/Generic/linear-dbg-value.ll

45 ↗

(On Diff #133781)

please remove all non-essential attributes. (everything in quotes)

[vsk]

test/CodeGen/Generic/linear-dbg-value.ll

29 ↗

(On Diff #133242)

Thanks! Could you also check that the second dbg.value() for %add is inserted right after the first one?

[jcranmer]

Hopefully this should be the last version.

[vsk]

This is in line with how we handle debug values in ScheduleDAGSDNodes::EmitSchedule (see ProcessSDDbgValues).

I don't see any issues with this patch but am not familiar enough with ScheduleDAGFast to give an explicit lgtm. I'll CC some folks who might be able to.

[aprantl]

test/CodeGen/Generic/linear-dbg-value.ll

29 ↗

(On Diff #134158)

It would be better to write this as

CHECK: DBG_VALUE ![[X:[0-9]+]], ![[EXPR:[0-9]+]]
CHECK: [[EXPR]] = !DIExpression()
...

etc. Hard-coding the metadata numbering is just asking for trouble :-)

[aprantl]

Seems fine otherwise, thanks!

[jcranmer]

How does this patch get checked in? I don't have commit privileges, so far as I know.

[aprantl]

I can commit this for you.

[aprantl]

There's something weird with the test output:

bb.3.for.body:
  successors: %bb.2(0x04000000), %bb.3(0x7c000000)

  %2:gr64 = PHI %8, %bb.1, %7, %bb.3
  %3:gr64 = PHI %0, %bb.1, %6, %bb.3
  %4:gr32 = PHI %11, %bb.1, %5, %bb.3
  %5:gr32 = ADD32rm %4, %2, 1, $noreg, 0, $noreg, implicit-def dead $eflags, debug-location !33 :: (load 4 from %ir.lsr.iv1, !tbaa !29)
  DBG_VALUE debug-use %5, debug-use $noreg, !14, !DIExpression(), debug-location !19
  DBG_VALUE debug-use %5, debug-use $noreg, !14, !DIExpression(), debug-location !19
  %6:gr64 = DEC64r %3, implicit-def $eflags, debug-location !21
  %7:gr64 = ADD64ri8 %2, 4, implicit-def dead $eflags, debug-location !21
  JE_1 %bb.2, implicit $eflags, debug-location !24
  JMP_1 %bb.3, debug-location !24

Why is this DBG_VALUE inserted twice?

[vsk]

@aprantl looks to me like there's one dbg_value inserted per dbg.value instruction referring to %add. I asked for this to be filechecked explicitly.

[aprantl]

@vsk just pointed out that the dbg.value was in the original input already. That's also a bug, but not in your patch :-)

llvm/trunk/lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp

 //===----- ScheduleDAGFast.cpp - Fast poor list scheduler -----------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This implements a fast scheduler.
 //
 //===----------------------------------------------------------------------===//
 
 #include "InstrEmitter.h"
 #include "ScheduleDAGSDNodes.h"
+#include "SDNodeDbgValue.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/CodeGen/SchedulerRegistry.h"
 #include "llvm/CodeGen/SelectionDAGISel.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/IR/DataLayout.h"

 ScheduleDAGLinearize::EmitSchedule(MachineBasicBlock::iterator &InsertPos) {
   InstrEmitter Emitter(BB, InsertPos);
   DenseMap<SDValue, unsigned> VRBaseMap;
 
   DEBUG({
       dbgs() << "\n*** Final schedule ***\n";
     });
 
-  // FIXME: Handle dbg_values.
   unsigned NumNodes = Sequence.size();
+  MachineBasicBlock *BB = Emitter.getBlock();
   for (unsigned i = 0; i != NumNodes; ++i) {
     SDNode *N = Sequence[NumNodes-i-1];
     DEBUG(N->dump(DAG));
     Emitter.EmitNode(N, false, false, VRBaseMap);
+
+    // Emit any debug values associated with the node.
+    if (N->getHasDebugValue()) {
+      MachineBasicBlock::iterator InsertPos = Emitter.getInsertPos();
+      for (auto DV : DAG->GetDbgValues(N)) {
+        if (DV->isInvalidated())
+          continue;
+        if (auto *DbgMI = Emitter.EmitDbgValue(DV, VRBaseMap))
+          BB->insert(InsertPos, DbgMI);
+        DV->setIsInvalidated();
+      }
+    }
   }
 
   DEBUG(dbgs() << '\n');
 
   InsertPos = Emitter.getInsertPos();
   return Emitter.getBlock();
 }
 

llvm/trunk/test/DebugInfo/X86/linear-dbg-value.ll

+; RUN: llc -stop-before=expand-isel-pseudos -pre-RA-sched=linearize < %s | FileCheck %s
+source_filename = "/Data/llvm/test/DebugInfo/X86/linear-dbg-value.ll"
+
+; Function Attrs: nounwind readonly uwtable
+define i32 @foo(i32* nocapture readonly %a, i32 %N) local_unnamed_addr #0 !dbg !6 {
+entry:
+  %cmp6 = icmp sgt i32 %N, 0, !dbg !11
+  br i1 %cmp6, label %for.body.preheader, label %for.cond.cleanup, !dbg !15
+
+for.body.preheader:                               ; preds = %entry
+  %wide.trip.count = zext i32 %N to i64
+  br label %for.body, !dbg !17
+
+for.cond.cleanup.loopexit:                        ; preds = %for.body
+  br label %for.cond.cleanup, !dbg !19
+
+for.cond.cleanup:                                 ; preds = %for.cond.cleanup.loopexit, %entry
+  %x.0.lcssa = phi i32 [ 0, %entry ], [ %add, %for.cond.cleanup.loopexit ]
+  ret i32 %x.0.lcssa, !dbg !19
+
+for.body:                                         ; preds = %for.body, %for.body.preheader
+; CHECK: ![[X:[0-9]+]] = !DILocalVariable(name: "x",
+; CHECK-LABEL: bb.3.for.body:
+; CHECK: DBG_VALUE {{.*}} ![[X]], !DIExpression()
+; CHECK: DBG_VALUE {{.*}} ![[X]], !DIExpression()
+  %indvars.iv = phi i64 [ %indvars.iv.next, %for.body ], [ 0, %for.body.preheader ]
+  %x.07 = phi i32 [ %add, %for.body ], [ 0, %for.body.preheader ]
+  %arrayidx = getelementptr inbounds i32, i32* %a, i64 %indvars.iv, !dbg !17
+  %0 = load i32, i32* %arrayidx, align 4, !dbg !17
+  %add = add nsw i32 %0, %x.07, !dbg !17
+  call void @llvm.dbg.value(metadata i32 %add, metadata !9, metadata !DIExpression()), !dbg !20
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1, !dbg !21
+  call void @llvm.dbg.value(metadata i32 %add, metadata !9, metadata !DIExpression()), !dbg !20
+  %exitcond = icmp eq i64 %indvars.iv.next, %wide.trip.count, !dbg !11
+  br i1 %exitcond, label %for.cond.cleanup.loopexit, label %for.body, !dbg !15
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+attributes #0 = { nounwind readonly uwtable }
+attributes #1 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+!llvm.ident = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 4.0.1 ", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "foo.c", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{!"clang version 4.0.1 "}
+!6 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 2, type: !7, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !8)
+!7 = !DISubroutineType(types: !2)
+!8 = !{!9}
+!9 = !DILocalVariable(name: "x", scope: !6, file: !1, line: 3, type: !10)
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DILocation(line: 4, scope: !12)
+!12 = !DILexicalBlockFile(scope: !13, file: !1, discriminator: 1)
+!13 = distinct !DILexicalBlock(scope: !14, file: !1, line: 4, column: 3)
+!14 = distinct !DILexicalBlock(scope: !6, file: !1, line: 4, column: 3)
+!15 = !DILocation(line: 4, scope: !16)
+!16 = !DILexicalBlockFile(scope: !14, file: !1, discriminator: 1)
+!17 = !DILocation(line: 5, scope: !18)
+!18 = distinct !DILexicalBlock(scope: !13, file: !1, line: 4, column: 31)
+!19 = !DILocation(line: 7, scope: !6)
+!20 = !DILocation(line: 3, scope: !6)
+!21 = !DILocation(line: 4, scope: !22)
+!22 = !DILexicalBlockFile(scope: !13, file: !1, discriminator: 3)