[PATCH] D83047: [LiveDebugValues] 2/4 Add instruction-referencing LiveDebugValues implementation

Fri Jul 24 07:37:27 PDT 2020

jmorse updated this revision to Diff 280460.
jmorse added a comment.

Hi,

Here's the correction for vlocJoin. Note that I've added tests in D83054 <https://reviews.llvm.org/D83054> as they're not runnable from this patch (ooof).

What the previous vlocJoin implementation failed to model was that we sometimes need to "propose" a variable value to then confirm it later. This is exactly what the VarLoc implementation is doing all the time [0], by allowing variable locations to be "true" until they're found to be "false", so that locations that are live-through a loop can be found. This doesn't happen for machine-value-numbers: we only work out what value-number a location has for that problem, while for variable-value joining we have to work out what the values are _and_ whether they're present at a common machine location to be a legitimate PHI.

I've addressed this by explicitly distinguishing "proposed" and "definite" values in the DbgValue "Kind" enumeration. Now:

- A "proposed" value is a machine-value-number, that is the variable value at a loop head if you ignore the backedges,
- A Def is any machine-value-number that is definitely the variable value in this live-in/live-out.

Proposed values are generated at loop heads whenever backedges don't agree on the live-in value. If we re-visit the loop head and find that the backedges now agree on the variable value (with proposed values), we've discovered a live-through value and it gets confirmed as a Def, which is propagated to all successors. If we never visit the loop head again though, the "proposed" value isn't considered a true variable location and is ignored during DBG_VALUE emission.

In terms of lattices, I think this splits each position into a proposed and definite element, but doesn't otherwise change the order of exploration. The test I've added (livedebugvalues_downgradefeedback_loop.mir in D83054 <https://reviews.llvm.org/D83054>) checks for this failure mode.

Two additional problems came up in the process:

- The exploration order we've previously been using isn't actually reverse-post-order!
- Moving down the lattice doesn't necessarily guarantee a loop head is re-visited later.

For the first item, take a look at the livedebugvalues_path_lengths.mir test I've added to D83054 <https://reviews.llvm.org/D83054>, and consider the machine value numbers. As the test comment explains, there's a long and short path from one block to another, which can lead to predecessors not having observed dominating changes that other predecessors have. This is fixed by, in the dataflow loops, adding immediate successors to the current worklist, and successors reached via backedges to the Pending worklist. This gives a stronger guarantee that all predecessors to a block will be visited, and will have observed changes to their own predecessors, before a block is visited.

The second item is demonstrated by livedebugvalues_insensitive_downgrade.mir in D83054 <https://reviews.llvm.org/D83054>, again considering machine value numbers. bb.1 generates a PHI value, which could be live-through the loop at bb.2, so we downgrade the live-in value at bb.2 to the PHI value from bb.1. bb.2 should be revisited to discover that the bb.1 PHI isn't live-through and a PHI should be generated at bb.2 -- however, the live-outs from both bb.2 and bb.3 do not change after the downgrade. This means bb.2 is never re-visited, and the PHI value at bb.2 is never discovered. I've fixed this by having any downgraded block booking itself to be re-visited on the next dataflow round.

There are a couple of final concerns with this implementation, firstly that there are now two ways of signalling the absence of a variable location. One is by not having a DbgValue present in the live-in/live-out map for the variable (a scenario caused by a DBG_VALUE $noreg in the transfer function), or, having vlocJoin fail to find a candidate value and create a "NoVal" DbgValue, which I've just added. I think I  still need to come up with an explanation of how that fits into the lattice exploration.

Secondly, the implementation doesn't _explicitly_ store where it is in the lattice exploration for each block. I'm 95% convinced that exploring it in reverse-post-order means we "store" stat
e in the form of earlier-in-RPO block live-outs, which could do with formalising and writing down somewhere.

[0] https://github.com/llvm/llvm-project/blob/b7f6ecf0c7d4ac86ed4983311d0501e75c659e25/llvm/lib/CodeGen/LiveDebugValues.cpp#L69

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

https://reviews.llvm.org/D83047

Files:
  llvm/lib/CodeGen/CMakeLists.txt
  llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
  llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.h

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D83047.280460.patch
Type: text/x-patch
Size: 121955 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200724/61bb103c/attachment.bin>