[PATCH] D56151: [DebugInfo] PR40010: Avoid register coalesing altering DBG_VALUE valuations

Wed Mar 27 05:21:48 PDT 2019

jmorse added a comment.

> Maybe ISel should have put the COPY to %0 after the last DBG_VALUE (now we got a dbg-use of %15 after the last non-dbg-use of %15). Or maybe it should have used %0 instead of %15 in that DBG_VALUE. Or maybe there should be one DBG_VALUE before the COPY using %15 and one after using %0.

Ugh, I remember debugging into this issue now (it then fell out of my short term memory). IIRC:

- All Values used between blocks get a vreg allocated,
- When the DAG is converted to MIR instructions, each instruction (with a def) gets its own vreg number
- CopyToReg nodes then copy from a local vreg to a cross-block vreg.

In various cases this means a DBG_VALUE gets attached to a block-local vreg that immediately gets copy-killed to the cross-block vreg, which this patch then has difficulties with. Given that this is, more or less, a design feature of SelectionDAG, I guess it should be considered An Issue (TM).

> Should this be seen as a fault in ISel? We could probably get a better result if ISel worked differently. But in general I think we allow dbg-uses outside the non-dbg-live-interval, so I do not think it really is a fault.

It seems to be part of the design, and I don't know SelectionDAG well enough to determine whether changing it would break assumptions elsewhere. Assuming my summary above is correct, a quick fix might be to prefer allocated vreg locations over even SDNodes when processing dbg.value intrinsics, but I haven't considered what else might go wrong.

The real trouble is that the DBG_VALUEs are correct while the MIR function is in SSA form, but when we leave it and consider liveness, they can become incorrect (or non-live at least).

> A late dbg-use (for an otherwise killed reg) is only unsound if the register allocator is using the allocated physical register for some other purpose. Isn't it a little bit early to detect that in the register coalescer? Maybe we need to take the register allocation into account if we want to make a better solution. Isn't LiveDebugVariables supposed to handle this?

Alas, I believe the register coalescer is doing some of the register allocators job: consider this example, produced by mangling test2 in the added regression test:

  bb.0.entry:
    successors: %bb.1(0x80000000)
    liveins: $rdi

    %1:gr32 = COPY $rdi
    %3:gr32 = MOV32r0 implicit-def dead $eflags

  bb.1.start.test2:
    successors: %bb.1(0x7c000000)

    %0:gr32 = PHI %50, %bb.1.start.test2, %3, %bb.0.entry
    %50:gr32 = XOR32rr %0, %1, implicit-def dead $eflags
    DBG_VALUE %0, $noreg, !3, !DIExpression(), debug-location !5
    JMP_1 %bb.1

Full file here: https://pastebin.com/HqkCB6Bi . Important feature is that the DBG_VALUE refers to %0, which is not live. Note that XOR32rr is a two-address instruction. If I run "llc -simplify-mir in.mir -start-before=phi-node-elimination -stop-before=simple-register-coalescing -o -", the value of %0 is preserved by the two-address-instruction pass through a copy, and the DBG_VALUE refers to that. But then if you change "stop-before" to "stop-after", you can see the coalescer merges %0 and %50, and the DBG_VALUE now refers to a live copy of the result of the XOR, not the input to it. (With this patch applied, I don't believe it does). Once the coalescer has run, there's no way to recover the original behaviour AFAIK.

LiveDebugVariables discards all non-live DBG_VALUEs [0]. I tried to fight against this for a bit, but then found bugs like this (this patch) cropping up.

~

Given that we've identified this now, it's probably best to keep this & the associated patches hanging until there's a fix for this behaviour (the local/cross-function vreg copying that is). Which is awkward, but it's ages until LLVM9 branches.

[0] https://github.com/llvm-mirror/llvm/blob/19a56211e133d6981fca86913ca6b97a701cee52/lib/CodeGen/LiveDebugVariables.cpp#L626

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56151/new/

https://reviews.llvm.org/D56151