Page MenuHomePhabricator

[WIP][DebugInfo] When sinking DBG_VALUEs, only insert undef DBG_VALUEs if sinking would re-order assignments
AbandonedPublic

Authored by jmorse on Mar 6 2019, 8:41 AM.

Details

Reviewers
None
Summary

This patch is a follow-up to D58238, where the discussion asked in what circumstances we needed to insert undef DBG_VALUEs into a block when sinking instructions. The answer was (to my mind) "only when it would re-order the appearance of variable assignments". This patch attempts to implement that.

I then realised that I'd misread some of Bjorns question there, so leaving this as a WIP with no reviewers until I'm certain this is a good idea.

The overall idea here is to keep a MachineBasicBlock::iterator that points at (or after) the last DBG_VALUE in a block. We can then identify whether DBG_VALUEs that are to be sunk, sink past the last one, which would mean assignments were re-ordered. In the worst case scenario we'll end up iterating over each basic block twice instead of once, which IMO is a better trade-off than anything that allocates memory.

If a set of DBG_VALUEs sink without any other DBG_VALUE being between them and the end of the block, for example:

blah:
    %0 = tosink...
    DBG_VALUE %0, "var1"
    %1 = inst1...
    DBG_VALUE %0, "var2"
    %2 = inst2...
    DBG_VALUE %0, "var3"
    br i1 %cond, label %dest, label %nope

and if "tosink" were sunk to another block, we would sink all those DBG_VALUEs into the destination block like this:

blah:
    %1 = inst1...
    %2 = inst2...
    br i1 %cond, ...
dest:
    %0 = tosink...
    DBG_VALUE %0, "var1"
    DBG_VALUE %0, "var2"
    DBG_VALUE %0, "var3"
    [...]
nope:
    DBG_VALUE undef, "var1"
    DBG_VALUE undef, "var2"
    DBG_VALUE undef, "var3"
    [...]

This reduces the number of sets of of different variable values you can see, because var1/var2/var3 all transition at the same time. This it the correct behaviour IMO because:

  • We can't cover the 'blah' block with %0 regardless
  • We're keeping var1/var2/var3 alive as long as possible in the 'blah' block

Wheras if there were a DBG_VALUE %2, "var4" after 'inst2', we would get:

blah:
    %1 = inst1...
    %2 = inst2...
    DBG_VALUE undef, "var1"
    DBG_VALUE undef, "var2"
    DBG_VALUE %2, "var4"
    br i1 %cond, ...
dest:
    %0 = tosink...
    DBG_VALUE %0, "var1"
    DBG_VALUE %0, "var2"
    DBG_VALUE %0, "var3"
    [...]
nope:
    DBG_VALUE undef, "var3"
    [...]

With var1/var2 leaving undef DBG_VALUEs in place in 'blah', to avoid earlier assignments to var1/var2 reordering with the assignment to var4.

I've split copy-propagation into its own function and applied it before sinking, to avoid awkward interactions with the "last DBG_VALUE in block" iterator. Any DBG_VALUE that can be copy-propagated doesn't need to be sunk anyway, and livedebugvalues should observe the sunk copy and keep the variable alive there.

Test changes:

  • postra-ignore-dbg-instrs.mir: the DBG_VALUE gets copy-prop'd now, and the test is only checking that the debug instruction doesn't affect codegen, so I've deleted the debug check line.
  • pr38952.mir: this is checking that non-immediately-following DBG_VALUEs get sunk. With this patch, the example gets copy-prop'd. I've changed the DBG_VALUE to refer to a subregister instead (which gets sunk), and test that not-immediately-following DBG_VALUEs get found.
  • machinesink.mir: as we're no longer re-ordering assignments, a sunk DBG_VALUE no longer leaves an undef DBG_VALUE behind.
  • postra-subreg-sink.mir: Two DBG_VALUEs no longer leave undef's behind, one only gets copy-prop'd, not sunk and copy-prop'd.
  • sink-leaves-undef.mir: insert a DBG_VALUE that would lead to ordering reassignment, to test that an undef DBG_VALUE is generated

Diff Detail