I believe there are a few places in the existing codebase where we intentionally drop fragments that could be updated to use this class; on the whole though, I approve of using a new class to differentiate between a variable fragment that happens to be the whole variable vs an explicit reference to the whole variable regardless of fragmentation.

Simple enough, LGTM.

Added a test to check that --target-run-args and DexCommandLine compose - needs to be a separate test file because of a few necessary differences in the file, which can't be fixed by using sed in the RUN line.

Stack LGTM, seems like a quite straightforward set of changes.

LGTM - I think not deleting debug intrinsics if there would be no other codegen changes is the right call for now as a simplest viable fix for the linked issue; hopefully this doesn't result in intrinsic bloat, but if it does that can be solved separately.

Broadly looks good, just a couple questions and a nit comment.

Does anyone have any good ideas on what a test for this looks like?

Small suggestion below, but LGTM with or without it.

LGTM!

Thanks for the changes, LGTM.

Nit, otherwise LGTM!

Looks broadly good to me, with some small suggestions.

Actually looking back over it I'd add further - FindValueForBlock isn't necessarily right because we're looking for values in the middle of the block, and that function will give us the value at the end only (which may be incorrect depending on the position of the debug value). I encountered this problem in MachineSSAUpdater, and the approach I took there was to add a ExistingValueOnly parameter to GetValue(InMiddle|AtEnd)OfBlock that performs all the same logic but returns empty if it would need to generate any code to provide a value. Happy to repeat that fix here!

In D140404#4109678, @jmorse wrote:

/me squints -- I think the idea here was that if SSAUpdater has a value at the end of the block we can use that and it won't cause new codegen; but then we're using GetValueInMiddleOfBlock, which will generate code in certain circumstances.

Changes LGTM!

Small point about the CallBr guard, but no other issues from my PoV!

In D133925#4040811, @dblaikie wrote:

I'll see if I can come up with a reproducer. Do you have an example of this from clang, rather than from handcrafted IR?

In D133925#4037971, @dblaikie wrote:

Hmm - shouldn't we still see localh emitted, but without a DW_AT_location - dropping the variable entirely seems incorrect? I guess just for this test case that happens because the variable isn't listed in the DISubprogram's variables list?

Rebased onto latest main, added unit tests for new convertToUndefExpression, and converted some existing tests to test for the new fully-undef debug values that SelectionDAG produces.

Rebased onto latest main, slightly updated and added unit tests for convertToNonVariadicExpression.

Rebased onto latest master, added unit tests for convertToVariadicExpression.

Rebased again now that the patch stack is ready to land.

Address comments (extracted MO selection for const debug operands, expanded comments in EmitDbgValue), add a function for setting a DIExpression "undef".

LGTM, thanks for the changes!

Minor comments but otherwise LGTM.

LGTM!

Simple and good, LGTM.

LGTM.

LGTM with the suggested "changes".

Simple enough, LGTM (but since this is part of a more substantial patch stack, leave a little time for others to have a look).

Largely LGTM, but with a potential error that needs looking at.

In D133927#4007888, @jmorse wrote:

Looks good, but with what feels like low test coverage: remind me, the plumbing required to track variadic locations in LiveDebugValues already landed, and we have tests to cover those new code paths, yes?

Rebased onto latest master; addressed review comments. The section of code that turned variadic expressions into non-variadic expressions so that they could be recovered as entry values has been extracted into a separate function in DebugInfoMetadata, and another instance of that logic in AsmPrinter has been updated to use this function. If you think that change should be rolled into a separate patch then I'm happy to do so!

Updated for review comments, None -> std::nullopt.

In D140412#4008377, @jmorse wrote:

Slightly twitchy about there being a zero LocIdx -- I think I tried to suppress there being any default-constructed LocIdxes out there. On the other hand, it comes with a sentinel value attached, so it's no big deal.

Not very familiar with this area, but it looks as though currently InstCombinerImpl::transformConstExprCastCall in InstCombineCalls.cpp will need updating as well. Specifically it may transform a CallBr instruction and insert a Cast for the return, for which it calls NewCall->getInsertionPointAfterDef() and asserts that the return is non-null. Not sure what the solution would be, but it might require dropping support for CallBr in transformConstExprCastCall if the return type is changed: there's already a block in there that bails out of transforming invoke or callbr instructions if they are used in a PHI node, since there's no place to insert the Cast in that case.

Apologies for taking a bit of time to get to this. My thoughts below, tl;dr it's probably better to just skip tracking spills for DBG_VALUE_LISTs in RegAllocFast.

Some notes about the DIArgList stuff, otherwise LGTM.

LGTM with minor comments.

Changes LGTM.

Changes all LGTM, except the one unaddressed comment about not using $noreg in loop-sink.ll.

Move some changes from an earlier patch into this one: Make entry value expressions containing only a leading DW_OP_LLVM_arg, 0 valid, as the changes in this patch enable such expressions. Also add the convertToVariadicExpression function, as this is the first patch in which it is used.

Rewrote the test to remove some incorrect behaviour; new test uses two different variables to compare direct/indirect and non-variadic/variadic separately. Also, add the missing variable equality check in the added MachineInstr function (the only caller already checks for variable equivalence, but the function would be misleading if it didn't do so as well!)

Address review comments, including removing the unnecessary call to MachineInstr::isIdenticalTo in MachineInstr::isEquivalentDbgInstr and also adding a missing check for getDebugLoc equality.

Some minor comments, but otherwise LGTM.