Page MenuHomePhabricator

[DebugInfo][DAG] Allow creation of DBG_VALUEs in blocks where the operand is not used

Authored by jmorse on Jan 14 2019, 10:56 AM.



This patch allows DBG_VALUEs to be created by SelectionDAGBuilder referring to Values that aren't used in the current basic block. This is done by allowing any Value that has a VReg allocated but no SDNode in this block, to be referred to as a DBG_VALUE of that VReg.

Observe the attached test case (IR only, the rest of the DebugInfo is lifted from a different test). Currently, the dbg.value of %1 will not generate a DBG_VALUE in the "nextbb" block because %1 isn't used there, and thus doesn't get an SDNode. Instead the debug record is left in SelectionDAG's DanglingDebugInfo across BBs until %1 is used. That's unfortunate for the user because the variables location range is un-necessarily shortened. I've seen many circumstances where inlining and CSE effectively hoists the computation of a value out of a block, leaving a dbg.value in this situation, to the inconvenience of the user.

We already have code in SelectionDAGBuilder that creates debug records for VRegs: this patch just allows it to happen for most Values (any Value used across BBs gets a VReg). Some observations:

  • We can be sure the produced DBG_VALUE will refer to the correct VReg def, because (I believe) SelectionDAG produces machine code still in SSA form,
  • There's a risk that SelectionDAG re-orders instructions, but that's no different for VReg debug records than SDNode debug records,
  • Some of the DBG_VALUEs generated by this change will be of dead / non-live VRegs, AFAIK this isn't something that LLVM currently cares too much about,
  • Some of the DBG_VALUES could also be debug use-before-defs, but that'll be the fault of other optimisation passes.

I've filtered out any Argument Values that have an associated VReg: otherwise DebugInfo/X86/sdag-split-arg.ll fails. Otherwise it seems DBG_VALUEs end up in one group in the middle of the BB in that test, rather than being interleaved with the argument copies. Filtering out Argument Values allows them to reach SelectionDAGBuilder::EmitFuncArgumentDbgValue where there's special-case behaviour.

(This patch doesn't currently lead to variable coverage improvement due to CodeGenPrepare::placeDbgValues juggling dbg.values, however this patch enables progress in the near-future. In fact, I suspect the use-case of this patch hasn't been encountered, due to placeDbgValues. See PR38754 for why it needs to go away).

See also r331182 / D46129 for the discussion when this VReg debug record facility was originally added to SelectionDAGBuilder.

Diff Detail


Event Timeline

jmorse created this revision.Jan 14 2019, 10:56 AM
jmorse set the repository for this revision to rL LLVM.Jan 14 2019, 11:03 AM

Interesting. If I remember correctly this is similar to something I played around with half a year ago (as a preparation before prohibiting CodeGenPrepare::placeDbgValues to move DBG_VALUE between basic blocks (or even to disable CodeGenPrepare::placeDbgValues completely)). I'll have a closer look at this when I'm at the office. I'll also try to remember if I had some specific reasons for not moving forward with my attempt at doing this - it has been bothering me that I never found the time to finalize that patch.

Seems okay to me, but it would be good to wait Björn's LGTM.

bjope accepted this revision.Jan 16 2019, 8:36 AM


PS. I found my old git branch where I played around with this, and I think that I never figured out that I needed the !isa<Argument> check. Maybe that is why I had mixed feeling about the result back in the days.

This revision is now accepted and ready to land.Jan 16 2019, 8:36 AM

Thanks for the reviews,

PS. I found my old git branch where I played around with this, and I think that I never figured out that I needed the !isa<Argument> check. Maybe that is why I had mixed feeling about the result back in the days.

It's a well hidden code path, and I suspect there's extra pain with argument DBG_VALUEs given examples such as [0], alas :/


This revision was automatically updated to reflect the committed changes.