This is an archive of the discontinued LLVM Phabricator instance.

debuginfo: Improve line info when translating a CaseBlock to SDNodes
ClosedPublic

Authored by frej on Aug 14 2017, 5:04 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

frej created this revision.Aug 14 2017, 5:04 AM

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 added inline comments.Aug 15 2017, 6:25 AM
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 added inline comments.Aug 15 2017, 10:36 AM
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 added inline comments.Aug 15 2017, 11:39 PM
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

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 accepted this revision.Aug 16 2017, 9:31 AM

LGTM with all outstanding issues addressed.

This revision is now accepted and ready to land.Aug 16 2017, 9:31 AM
frej updated this revision to Diff 111477.Aug 17 2017, 1:37 AM

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 added a comment.Aug 17 2017, 1:37 AM

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.

This revision was automatically updated to reflect the committed changes.