Ping - this one has had a lot of discussion, and I think all of the substantial concerns have been addressed; are there any further comments?

Ping.

Ping.

Ping - any further comments?

Fixed issue in which DBG_VALUE_LIST expressions were being emitted without subregister masking.

Rebased onto recent master (resolving clashes with the DW_OP_implicit_ptr patch); rewrote dbg-value-list-spill.mir test.

Rebased onto recent master, fixed test failures.

Rebased onto recent master, address review comment.

Rebased onto recent master.

Rebased onto recent master, minor change to Args storage (std::vector -> SmallVector) and added args_begin() and args_end().

Rebased onto recent master, added smarter iterator for variable location operands.

Rebased onto recent master.

Rebased onto recent master, fixed test failures.

Rebased onto recent master, fixed some test failures.

Rebased onto recent master, fixed up MachineSink implementation.

Rebased onto recent master (993c488ed), addressed review comments (except for the question about dbg-value-list-spill.mir).

In D88175#2450058, @aprantl wrote:

Although — no that I read the reply — it looks like the MDTuple question hasn't actually been answered ;-)

In D83895#2225768, @Orlando wrote:

I'm hoping you can clarify something about addDefsFromCopies for me. IIUC without the patch we are adding defs from copies at kill points if possible. And with the patch a def is only added on the _first_ kill. Is that right?

In D83890#2177846, @djtodoro wrote:

@StephenTozer Thanks for addressing the comments! Can you please share with us if you have any measurements with the respect to build time increase (e.g. on clang build itself) with this patch? I think that this patch is the only one from the stack that could give us potential performance issues.

Rebased onto recent master, no functional changes; moved the salvageDebugInfoImpl refactoring into a separate NFC patch; addressed outstanding review comments.

Rebased onto recent master; no functional changes.

Rebase onto recent master, no functional changes.

Rebase onto recent master; no functional changes except applying the existing changes to the new InstrEmitter::EmitDbgInstrRef function.

Rebase onto recent master, no functional changes from rebase. Minor changes to findDbgUsers/findDbgValues to handle DIArgLists in which the same value appears more than once.

Rebase onto recent master; no functional changes from rebase. Minor change to DbgVariableIntrinsic::setUndef to handle DIArglists in which the same value appears more than once.

Rebase onto recent master; minor non-functional changes (renamed "ParseX()" functions to "parseX()").

Rebased onto recent master, no changes.

Rebased onto recent master; no functional changes from rebase.

Rebased to recent master, no actual changes.

Rebased onto recent master; no functional changes, minor adjustments in DwarfDebug::emitDebugLocValue.

Rebased onto recent master.

Rebased onto recent master.

Fixed a pair of bugs; 1) added DIArgList support to the ValueMapper so that DIArgList arguments are properly updated during inlining, and 2) prevented RAUW from setting DIArgList arguments to null (now uses an UndefValue instead).

In D91722#2404937, @djtodoro wrote:

Thanks!! So, this should be the most important one :) I would really like to see some measurements (e.g. llvm-locstats) before/after this patch.
Have you tried building llvm-project itself with this?

In D82363#2405903, @jmorse wrote:

@StephenTozer what's the status of the latest discussions on this patch? As far as I understand it:

• DBG_VALUE may go away, but that'll be in another patch,
• There was some conclusion to whether all DBG_VALUE_LISTs should be stack_values too, can't remember what, on the mailing list,
• This patch, adding the plumbing for the DBG_VALUE_LIST, would be ready to land.

I've added a test for the symptom that revealed this bug in llvm-mca. I'm also writing a unit test for AsmLexer that tests the underlying behaviour by verifying that CommentConsumers will not be sent characters that are not part of the line comment, since the problem is not specific to llvm-mca (although it's the only place that has seen an error so far, as far as I can tell).

LGTM, this new approach looks solid. As per usual, wait for further approval from someone with more authority in this area.

In D88593#2312053, @aprantl wrote:

Are you going to do GlobalISel, too?

• Fixed a few minor errors, including a linux compile error
• Rebased onto latest master, moving changes into VarLocBasedImpl.cpp.

Add documentation for DBG_VALUE_LIST and DW_OP_LLVM_arg.

LGTM, though recommend waiting for a more experienced reviewer to give approval.

I think wait for someone with more experience in this area to give approval, but aside from the outstanding comments, LGTM.

LGTM. I think the Implicit flag idea might be able to be folded into this patch without much issue, but it's not urgent and can definitely be shifted to another patch (or even just left as a TODO). Should probably be looked at by others with experience in the area, but I think most of the comments are probably going to be for the later patches in this stack.

Updated changes for latest master (there may be some lurking unseen errors, still trying to flush any out), addressed some review comments. Right now, I think the only thing holding this patch up is the discussion on the llvm-dev mailing list about using DBG_VALUE_LIST as the default for all debug values. If anyone on this review has any input for that, please go ahead and reply to the chain - I think that the latest proposal I put forward should satisfy everyone's requirements, but I don't want to push ahead without being sure of that.

In D82363#2217205, @aprantl wrote:

In a normal year I would have suggested a round table at the upcoming LLVM dev meeting, but this time around we'll have to improvise :-)
In the past, a post to LLVM-dev with (it does not have to be a fully fleshed out RFC at all) was always a good way to get a discussing with all stakeholders started.

Add test to confirm that addMachineRegExpression is being correctly used for DBG_VALUE_LIST regs; replace if-else chain with switch in AsmPrinter::emitDebugValueComment.

Add a simple unit test for the new function.

Remove unused MF attributes.

Remove unused data from tests, add/modify comments, move VarVec declaration up, fix error in duplicated-op-removal.

In D83495#2158601, @aprantl wrote:

Could we reduce complexity by entirely replacing DBG_VALUE with DBG_VALUE_LIST, *, DW_OP_arg 0, *?

Rename appendOpsToLoc->appendOpsToArg to fit the rest of the names, update isUndefDebugValue check to check all operands (pulled ahead from a patch further up the stack).

Remove useless const, add rename that somehow escaped last diff.