In D85741#2226310, @aprantl wrote:

Oh, one important thing: Can you add a section to the .rst documentation with a MIR example that explains what a DBG_VALUE_REF is / what it's used for, etc?

Revise some dodgy comments, cheers!

Address the loop matter I mention at the end of the review summary -- I've convinced myself that we only need to validate very simple loops in this context, because LLVMs tail-duplicator doesn't substantially rewrite functions, it only duplicates bits of it.

Address comments; add a test for creating DBG_PHIs for PHIs that happen in spill slots.

Thanks to all the reviewers in this series for their time!

NB: landing now-ish as recent feedback is all nits, and in the "main" patch (D83047) we've come to the conclusion that this should land guarded by an experimental switch (which is what this patch does).

Rebase to account for some whitespace differences.

Early-exit to avoid un-necessary indentation.

Remove some needless comments and some needless curly braces.

In D83047#2226512, @aprantl wrote:

Since I'm also dealing with some cognitive overload at the moment, I'm pragmatically going to agree!

Rebase for rG60433c63acb7 touching something near this patch, no real change.

Add a test for the driver -Xclang option. As far as I understand this, we're just checking that the switch is accepted by the driver, not that it does anything in particular (please correct me if I'm wrong).

Address @djtodoro 's comments (remove header, add C++ indicator, clang-format).

Add another call to substituteDebugValuesForInst that I missed (line 681).

In D83047#2208102, @vsk wrote:

Hi @jmorse, thanks for the updates on the DbgValue::{Proposed,NoVal} scheme.

I'm having trouble paging in enough of the DBG_INSTR_REF patch queue to continue giving meaningful review. I'm not sure the traditional review process is a great fit for this, since the changes are at a prototype stage. Perhaps we might be better served by landing things as they are, as a separate code path guarded behind a cl::opt? This would allow more people to test and contribute patches. Wdyt? (cc @aprantl @davide)

NB: on the topic of LowerDbgDeclare, it was discussed extensively in PR34136 (CC @rnk) , and there's actually a plan for eliminating its behaviour there [0] from about two years ago, which we were eventually going to look at (CC @chrisjackson).

No real additional comments on this or the 2nd patch -- for further progress though I think you need to stimulate some discussion in the llvm-dev "DW_OP_implicit_pointer design/implementation in general" thread, to convince David / Adrian that this patch series addresses their concerns from the earlier discussion.

I wrote:

were this series to land today, there would be some variable locations dropped

In D85756#2211044, @aprantl wrote:

Are there other targets that also need to be updated?

Switch a loop to being ranged-based.

In D83047#2208102, @vsk wrote:

I'm having trouble paging in enough of the DBG_INSTR_REF patch queue to continue giving meaningful review. I'm not sure the traditional review process is a great fit for this, since the changes are at a prototype stage. Perhaps we might be better served by landing things as they are, as a separate code path guarded behind a cl::opt? This would allow more people to test and contribute patches. Wdyt? (cc @aprantl @davide)

Here's the latest revision; in this change I've suppressed use of the "zero LocIdx": all location indexes now mean the same thing. I've done this by scattering Optional around a variety of places. There's still one rough edge, which is that the IndexedMaps in MLocTracker want a reserved value to signify a mapping that doesn't exist -- I've added an "illegal" maximally-valued LocIdx for that, which is only ever generated inside the IndexedMap. This too could be eliminated by using a std::map instead, if we're aiming for complete clean-ness at this stage.

I see what seem to be changes to codegen here (see inline comments), is that intentional? And if so, why is that needed for DW_OP_LLVM_explicit_pointer to work?

Suggested on llvm-dev: I reckon you also need to add logic to the ConvertDebugDeclareToDebugValue helpers too, or otherwise factor them in. This patch only covers the two fast-cases that mem2reg considers, single store allocas and single block allocas, I believe (98%) everything else goes through ConvertDebugDeclareToDebugValue.

Could you elaborate on why module-level information needs to be preserved? As mentioned on llvm-dev, I think you might have missed a place where promotion from stack to SSA/values happens -- IMHO we shouldn't add module-level debuginfo-tracking unless it's absolutely necessary.

I sense this is the bulk of the work; and it looks good. I've posted on llvm-dev asking whether the creation of artificial variables could instead be deferred til DWARF emission. That'll avoid anything having to deal with a new form of DBG_VALUE, and us having to think about their lifetimes. Plus, there's greater scope for de-duplication, as I understand it this patch will generate a new artificial variable for each dbg.value, for each level of indirection.

Similar story to the parent patch, could you put a file-comment into the test explaining it's a round-trip test?

Hi @alok, would you be able to re-title the reviews in this stack to indicate what's in the patch itself? That'll make it easier to navigate.

Misery: unfortunately diff 280460 undid the contents of the previous revision (adding isPHI helpers etc). This latest update restores that work -- please diff between this update (280920) and 280205 to see the vlocJoin rework. Sorry for the extra bother.

Here's the correction for vlocJoin. Note that I've added tests in D83054 as they're not runnable from this patch (ooof).

Add three new tests of interesting (TM) LiveDebugValues input, which I'll refer to in D83047:

[Bah, comments didn't get submitted]

Thanks for all the input -- this update addresses more of the feedback so far, although I've left a few things for future update(s). As mentioned above, there's some sketchyness with vlocJoin, which I now have a fix for, which I'll hopefully fold into this revision today or tomorrow.

Should be a matter of

(Drive-by review) Thanks for the patch -- it looks good, and generating legal machine code is definitely better than illegal,

On the topic of this implementation in general: the more I look at the vlocJoin method, the more I'm convinced it's incorrect. While it does produce an identical binary when stage2 building clang, I think it's only correctly dealing with the kind of control flow that C/C++ produces. If you expose it to weirder control flow, like these MIR files:

On the topic of entry values, I believe the value based tracking should make identifying entry values trivial -- all entry value numbers will be identifiable (as a ValueIDNum) with "BlockNo=0" and "InstNo="0", indicating a value defined before the first instruction of the first block. Ideally, the logic would be after machine-value-numbers and variable-values are propagated, when final locations are picked, and would look like this:

• This variables value is an entry value (BlockNo=0,InstNo=0),
• That value isn't available in any machine location right now
• I will emit a DBG_VALUE containing a DW_OP_LLVM_entry_value expression.

Right now I've got some extra work on this (InstrRefBasedLDV) patch to be done, more on that shortly, and a patch series adding some initial DBG_INSTR_REF support to upload. I should be able to prod the entry-value situation after that.

In D83047#2133722, @vsk wrote:

Thanks for this, Jeremy. It'll take me multiple passes to page all of this in. I hope to get to the core algorithm changes in my next review. In the interest of getting some feedback to you sooner rather than later, I've included some minor suggestions and questions inline.

• Replace a std::pair with a DbgValueProperties class,
• Replace some Densemaps of LocIdx with std::map, for initial cleanliness,
• Rename ValueRec to DbgValue and make two exclusive fields a union,
• Use an Optional<ValueIDNum> instead of in-band signalling when it's an invalid result,
• Address additional assorted feedback.

(Rebase, only affecting LiveDebugValues.cpp)

(Rebasing, only affects LiveDebugValues.h)

Move include to start of file; was on autopilot sorry.

Thanks for the feedback,

clang-format, add file headers, address some review comments

In D83236#2133902, @dblaikie wrote:

Could the algorithm be changed to do validThroughout of all variable fragments in a single pass together?

NB: in D83054 I've added livedebugvalues_many_loop_heads.mir which contains a worked example (aka pathological input) that exercises the lattice-descent code for machine value numbers. It's definitely the ugliest part.

Paul wrote:

And yet, the variable was allocated to a register, and the variable's location information pointed to the correct instruction range.
Inadequacies in our ability to represent the scope properly shouldn't cause us to eliminate *correct* location information for variables.

FWIW: I'd enjoy this patch landing. AFAIUI there's no meaningful information communicated to the DWARF consumers by out-of-scope ranges (even if it indicates a compiler bug somewhere), and we may as well save them disk space and debugger-load-time.

For the record, this all LGTM.

(Ah yeah, Tom is away for a bit),