debuginfo: Improve line info when translating a CaseBlock to SDNodes

[frej]

Summary

The SelectionDAGBuilder translates various conditional branches into
CaseBlocks which are then translated into SDNodes. If a conditional
branch results in multiple CaseBlocks only the first CaseBlock is
translated into SDNodes immediately, the rest of the CaseBlocks are
put in a queue and processed when all LLVM IR instructions in the
basic block have been processed.

When a CaseBlock is transformed into SDNodes the SelectionDAGBuilder
is queried for the current LLVM IR instruction and the resulting
SDNodes are annotated with the debug info of the current
instruction (if it exists and has debug metadata).

When the deferred CaseBlocks are processed, the SelectionDAGBuilder
does not have a current LLVM IR instruction, and the resulting SDNodes
will not have any debuginfo. As DwarfDebug::beginInstruction() outputs
a .loc directive for the first instruction in a labeled
block (typically the case for something coming from a CaseBlock) this
tends to produce a line-0 directive.

This patch changes the handling of CaseBlocks to store the current
instruction's debug info into the CaseBlock when it is created (and the
SelectionDAGBuilder knows the current instruction) and to always use
the stored debug info when translating a CaseBlock to SDNodes.

[aprantl]

otherwise this changes looks reasonable to me.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

241

FYI, the preferred way of marking up this comment would be
/// The debug location ...

llvm/test/CodeGen/X86/debugloc-no-line-0.ll

1

rather than compiling all the way down to assembler, could this be done with -stop (e.g. -stop-before-regalloc or -stop-after-livedebugvariables) so the test case becomes less fragile?

[frej]

llvm/test/CodeGen/X86/debugloc-no-line-0.ll

1

I had a look at checking the mir-code, this exceeds my FileCheck-fu. Instead of just checking for the absence of a ".loc 1 0"-line I would have to check that each "real" instruction has a debug-location. Any pointers on how I should go about this? How do I distinguish "real" instructions from the IMPLICIT_DEFs, labels and successor info? If I hard-code the expected instructions I would expect it to be much more brittle than the current version checking the assembly.

[aprantl]

llvm/test/CodeGen/X86/debugloc-no-line-0.ll

1

Could you paste the MIR output with and without your patch? I'm happy to help with crafting the CHECKs.

[frej]

llvm/test/CodeGen/X86/debugloc-no-line-0.ll

1

Sure!

Without the patch

name:            main
alignment:       4
exposesReturnsTwice: false
legalized:       false
regBankSelected: false
selected:        false
tracksRegLiveness: true
registers:
  - { id: 0, class: gr32, preferred-register: '' }
  - { id: 1, class: gr32, preferred-register: '' }
  - { id: 2, class: gr32, preferred-register: '' }
  - { id: 3, class: gr32, preferred-register: '' }
  - { id: 4, class: gr32, preferred-register: '' }
  - { id: 5, class: gr32, preferred-register: '' }
liveins:
frameInfo:
  isFrameAddressTaken: false
  isReturnAddressTaken: false
  hasStackMap:     false
  hasPatchPoint:   false
  stackSize:       0
  offsetAdjustment: 0
  maxAlignment:    0
  adjustsStack:    false
  hasCalls:        false
  stackProtector:  ''
  maxCallFrameSize: 4294967295
  hasOpaqueSPAdjustment: false
  hasVAStart:      false
  hasMustTailInVarArgFunc: false
  savePoint:       ''
  restorePoint:    ''
fixedStack:
stack:
constants:
body:             |
  bb.0.entry:
    successors: %bb.1.sw.bb(0x40000000), %bb.4.entry(0x40000000)

    %1 = IMPLICIT_DEF
    %0 = COPY %1, debug-location !11
    %3 = IMPLICIT_DEF
    %2 = COPY %3, debug-location !11
    %2 = SUB32ri8 %2, 1, implicit-def %eflags, debug-location !11
    JE_1 %bb.1.sw.bb, implicit %eflags, debug-location !11
    JMP_1 %bb.4.entry, debug-location !11

  bb.4.entry:
    successors: %bb.2.sw.bb2(0x40000000), %bb.3.sw.epilog(0x40000000)

    %4 = COPY %0
    %4 = SUB32ri8 %4, 2, implicit-def %eflags
    JE_1 %bb.2.sw.bb2, implicit %eflags
    JMP_1 %bb.3.sw.epilog

  bb.1.sw.bb:
    successors: %bb.3.sw.epilog(0x80000000)

    JMP_1 %bb.3.sw.epilog, debug-location !12

  bb.2.sw.bb2:
    successors: %bb.3.sw.epilog(0x80000000)

    JMP_1 %bb.3.sw.epilog, debug-location !14

  bb.3.sw.epilog:
    %5 = MOV32ri 4711
    %eax = COPY %5, debug-location !15
    RETQ implicit %eax, debug-location !15

With the patch:

name:            main
alignment:       4
exposesReturnsTwice: false
legalized:       false
regBankSelected: false
selected:        false
tracksRegLiveness: true
registers:
  - { id: 0, class: gr32, preferred-register: '' }
  - { id: 1, class: gr32, preferred-register: '' }
  - { id: 2, class: gr32, preferred-register: '' }
  - { id: 3, class: gr32, preferred-register: '' }
  - { id: 4, class: gr32, preferred-register: '' }
  - { id: 5, class: gr32, preferred-register: '' }
liveins:
frameInfo:
  isFrameAddressTaken: false
  isReturnAddressTaken: false
  hasStackMap:     false
  hasPatchPoint:   false
  stackSize:       0
  offsetAdjustment: 0
  maxAlignment:    0
  adjustsStack:    false
  hasCalls:        false
  stackProtector:  ''
  maxCallFrameSize: 4294967295
  hasOpaqueSPAdjustment: false
  hasVAStart:      false
  hasMustTailInVarArgFunc: false
  savePoint:       ''
  restorePoint:    ''
fixedStack:
stack:
constants:
body:             |
  bb.0.entry:
    successors: %bb.1.sw.bb(0x40000000), %bb.4.entry(0x40000000)

    %1 = IMPLICIT_DEF
    %0 = COPY %1, debug-location !11
    %3 = IMPLICIT_DEF
    %2 = COPY %3, debug-location !11
    %2 = SUB32ri8 %2, 1, implicit-def %eflags, debug-location !11
    JE_1 %bb.1.sw.bb, implicit %eflags, debug-location !11
    JMP_1 %bb.4.entry, debug-location !11

  bb.4.entry:
    successors: %bb.2.sw.bb2(0x40000000), %bb.3.sw.epilog(0x40000000)

    %4 = COPY %0, debug-location !11
    %4 = SUB32ri8 %4, 2, implicit-def %eflags, debug-location !11
    JE_1 %bb.2.sw.bb2, implicit %eflags, debug-location !11
    JMP_1 %bb.3.sw.epilog, debug-location !11

  bb.1.sw.bb:
    successors: %bb.3.sw.epilog(0x80000000)

    JMP_1 %bb.3.sw.epilog, debug-location !12

  bb.2.sw.bb2:
    successors: %bb.3.sw.epilog(0x80000000)

    JMP_1 %bb.3.sw.epilog, debug-location !14

  bb.3.sw.epilog:
    %5 = MOV32ri 4711
    %eax = COPY %5, debug-location !15
    RETQ implicit %eax, debug-location !15

[aprantl]

Looking at the diff, it seems reasonable to check that all instructions in bb4 now have a debug-location annotation, and that the annotation is identical to the one on e.g., the jmp to bb4.

For example:

CHECK: JMP{{.*}}%bb.4.entry, debug-location ![[JUMPLOC:[0-9]+]]
CHECK: bb.4.entry:
CHECK-NOT: :
CHECK: JE{{.*}}debug-location ![[JUMPLOC]]
CHECK-NOT: :
CHECK: JMP{{.*}}debug-location ![[JUMPLOC]]

[aprantl]

LGTM with all outstanding issues addressed.

[frej]

Updated according to review comments. I had to add a CHECK: successors: after CHECK: bb.4.entry: in order to not crash on the successor info.

[frej]

Thanks, that's a smart way of doing it, although I had to insert a CHECK: successors: after CHECK: bb.4.entry: for it to not fail on the successor info.

I have updated the patch, I don't have commit rights so I'm depending on your charity for getting it committed.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

     CaseBlock(ISD::CondCode cc, const Value *cmplhs, const Value *cmprhs,
               const Value *cmpmiddle, MachineBasicBlock *truebb,
               MachineBasicBlock *falsebb, MachineBasicBlock *me,
+              SDLoc dl,
               BranchProbability trueprob = BranchProbability::getUnknown(),
               BranchProbability falseprob = BranchProbability::getUnknown())
         : CC(cc), CmpLHS(cmplhs), CmpMHS(cmpmiddle), CmpRHS(cmprhs),
-          TrueBB(truebb), FalseBB(falsebb), ThisBB(me), TrueProb(trueprob),
-          FalseProb(falseprob) {}
+          TrueBB(truebb), FalseBB(falsebb), ThisBB(me), DL(dl),
+          TrueProb(trueprob), FalseProb(falseprob) {}
 
     // CC - the condition code to use for the case block's setcc node
     ISD::CondCode CC;

     // ThisBB - the block into which to emit the code for the setcc and branches
     MachineBasicBlock *ThisBB;
 
+    // DL - the debug location of the instruction this CaseBlock was
+    // produced from.
+    SDLoc DL;
+
     // TrueProb/FalseProb - branch weights.
     BranchProbability TrueProb, FalseProb;
   };

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

       }
 
       CaseBlock CB(Condition, BOp->getOperand(0), BOp->getOperand(1), nullptr,
-                   TBB, FBB, CurBB, TProb, FProb);
+                   TBB, FBB, CurBB, getCurSDLoc(), TProb, FProb);
       SwitchCases.push_back(CB);
       return;
     }

   // Create a CaseBlock record representing this branch.
   ISD::CondCode Opc = InvertCond ? ISD::SETNE : ISD::SETEQ;
   CaseBlock CB(Opc, Cond, ConstantInt::getTrue(*DAG.getContext()),
-               nullptr, TBB, FBB, CurBB, TProb, FProb);
+               nullptr, TBB, FBB, CurBB, getCurSDLoc(), TProb, FProb);
   SwitchCases.push_back(CB);
 }
 

 
   // Create a CaseBlock record representing this branch.
   CaseBlock CB(ISD::SETEQ, CondVal, ConstantInt::getTrue(*DAG.getContext()),
-               nullptr, Succ0MBB, Succ1MBB, BrMBB);
+               nullptr, Succ0MBB, Succ1MBB, BrMBB, getCurSDLoc());
 
   // Use visitSwitchCase to actually insert the fast branch sequence for this
   // cond branch.

                                           MachineBasicBlock *SwitchBB) {
   SDValue Cond;
   SDValue CondLHS = getValue(CB.CmpLHS);
-  SDLoc dl = getCurSDLoc();
+  SDLoc dl = CB.DL;
 
   // Build the setcc now.
   if (!CB.CmpMHS) {

         }
 
         // The false probability is the sum of all unhandled cases.
-        CaseBlock CB(CC, LHS, RHS, MHS, I->MBB, Fallthrough, CurMBB, I->Prob,
-                     UnhandledProbs);
+        CaseBlock CB(CC, LHS, RHS, MHS, I->MBB, Fallthrough, CurMBB,
+                     getCurSDLoc(), I->Prob, UnhandledProbs);
 
         if (CurMBB == SwitchMBB)
           visitSwitchCase(CB, SwitchMBB);

 
   // Create the CaseBlock record that will be used to lower the branch.
   CaseBlock CB(ISD::SETLT, Cond, Pivot, nullptr, LeftMBB, RightMBB, W.MBB,
-               LeftProb, RightProb);
+               getCurSDLoc(), LeftProb, RightProb);
 
   if (W.MBB == SwitchMBB)
     visitSwitchCase(CB, SwitchMBB);

llvm/test/CodeGen/X86/debugloc-no-line-0.ll

+; RUN: llc -O0 -mtriple=x86_64-unknown-linux-gnu -o - %s | FileCheck %s
+define i32 @main() !dbg !12 {
+entry:
+  %add = add nsw i32 undef, 1, !dbg !16
+  switch i32 %add, label %sw.epilog [
+    i32 1, label %sw.bb
+    i32 2, label %sw.bb2
+  ], !dbg !17
+
+sw.bb:                                            ; preds = %entry
+  br label %sw.epilog, !dbg !20
+
+sw.bb2:                                           ; preds = %entry
+  br label %sw.epilog, !dbg !22
+
+sw.epilog:                                        ; preds = %sw.bb2, %sw.bb, %entry
+  ret i32 4711, !dbg !23
+}
+; This code should not result in any machine code where we have an
+; unknown source line, until after the epilog label.
+; CHECK-NOT: .loc 1 0
+; CHECK: %sw.epilog
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!8, !9}
+!llvm.ident = !{!11}
+
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, emissionKind: FullDebug, enums: !4)
+!3 = !DIFile(filename: "foo.c", directory: ".")
+!4 = !{}
+!7 = !DIBasicType(name: "int", size: 16, encoding: DW_ATE_signed)
+!8 = !{i32 2, !"Dwarf Version", i32 4}
+!9 = !{i32 2, !"Debug Info Version", i32 3}
+!11 = !{!"clang"}
+!12 = distinct !DISubprogram(name: "main", scope: !3, file: !3, line: 4, type: !13, isLocal: false, isDefinition: true, scopeLine: 5, flags: DIFlagPrototyped, isOptimized: false, unit: !2, variables: !4)
+!13 = !DISubroutineType(types: !14)
+!14 = !{!7}
+!16 = !DILocation(line: 6, column: 13, scope: !12)
+!17 = !DILocation(line: 6, column: 3, scope: !12)
+!19 = distinct !DILexicalBlock(scope: !12, file: !3, line: 7, column: 5)
+!20 = !DILocation(line: 10, column: 7, scope: !19)
+!22 = !DILocation(line: 13, column: 7, scope: !19)
+!23 = !DILocation(line: 24, column: 1, scope: !12)