Adrian wrote:

I don't think this is good.

In D67500#1668097, @aprantl wrote:

I'm assuming that at the beginning all in-locations are unknown and only when all incoming basic blocks have outgoing locations, join() adds new incoming locations to be processed.

clang-format patch

Address feedback, twiddle various things.

In D67393#1666272, @aprantl wrote:

First, DBG_VALUEs aren't necessarily source-level assignments *before* LiveDebugValues either, they update the SSA value that a (fragment) of a source-level variable can be found in, but that SSA value could have been created by the compiler and has not necessarily any relation to a source-level assignment (think about salvageDebugInfo, for example).

In D67393#1665380, @aprantl wrote:

Do you think we should avoid this confusion by calling them DBG_LOC or something else after LiveDebugValues to embrace the semantic difference?

(Ninja-edit: this moves transfer-insertion to before flushPendingLocs. flushPendingLocs may manipulate VarLocs that refer to not-yet-inserted DBG_VALUEs as their sources).

Expand a comment, explaining why we terminate overwritten spill-locations during LiveDebugValues, instead of leaving it until later.

Enjoyable leaps in variable-coverage / bytes-coverage on the far right of these graphs:

Delete a vector that isn't used by anything.

Nix an un-necessary else.

LGTM, thanks for digging into this!

Looking good; a couple of extra nits, then it's ready to go.

Update a comment, we don't terminate variables, instead we close locations.

Revise some comments & assertions.

This update simplifies & flattens the copy-forwarding logic as suggested. Rather than trying to select the condition where copy-forwarding is valid, instead continue around the loop whenever a precondition isn't met.

A case of mistaken identity meant I linked to this review in commit r370648, the real review for that commit is D58450, sorry for that extra noise.

Committed in https://reviews.llvm.org/rL370648

In D66941#1651703, @aprantl wrote:

Are all DBG_VALUEs pointing to spill slots created by LiveDebugValues and is this indicative of a bug elsewhere,

In D66942#1651751, @aprantl wrote:

Is there a big drop in variable locations with this (i.e., an improvement in accuracy)?

In D66663#1647741, @aprantl wrote:

This patch looks really elegant. Does the removal of the initialization phase have any adverse effect on the performance?

This update removes the "initialisation" phase that was happening in LiveDebugValues, and doing so fixes the original problem I was trying to solve, horray.

In D66663#1643786, @aprantl wrote:

Location of !103 changes in the loop block to constant-zero, which should later invalidate the in-location for the loop through the backedge, like it does in the "foo" function,

When the set out OutLocs changes (and in this case it changes from [crumpets=⊥] to [crumpets=0]) all successor blocks (in this case also the block itself because of the back edge) should be added to worklist automatically. Why isn't this happening?

However, the out-locations of the loop block are not changed as the result of propagation, therefore the loop block is never re-visited.

That I don't understand yet. They change from "no info" -> "some info". What am I missing?

No problemo

Remove an unnecessary change.

Update: I noticed that in moving transferTerminatorInst, I'd changed the "process" method to always return false, so I've removed its return code.

Adrian wrote:

Stepping back a bit: Is the result of SROA multiple smaller allocas?

If SROA is only creating new allocas, then describing them with dbg.declares should do no harm, since a later call to LowerDbgDeclare would truncate their range by inserting new dbg.values at every load. But if SROA is inserting the load, we do need to make sure that the loaded value is tracked by a dbg.value (potentially in addition to tracking the alloca with a dbg.declare). So I guess my question is, where are loads for the SROAed allocas generated?

Indeed, this'll need a MIR test for the BranchFolding pass to check for future regressions, and to demonstrate that this patch fixes the bug report. There are some MIR test examples in the llvm/test/CodeGen/MIR/X86 directory.

Adjust comment wordings

Looking good, and this produces the ~3% increase in variable locations in clang-3.4 builds like the previous revisions did too. It looks like the two xfailed tests are testing for the behaviour you're explicitly disabling: it's probably better to just delete them, as this is a deliberate decision to change that behaviour.

/Really/ avoid nondeterminism this time.

Sorry for the long delay --

Thanks for all the reviews!

Rebase onto the crash-fixing patch in D66284. This patch now removes the crash-fixing workaround that salvages fragments from spilt expressions, in favour of a more generalised method to access the original unspilt expression. I've edited the details of that into the patch summary at the top.