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).

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.

Remove old comments from tests.

Address latest review comments.

Update: Duplicate registers in the debug operands could be an issue with the previous patch version, and duplicates aren't easy to prevent, so always treat DBG_VALUE_LISTs as potentially having them.

Apologies for the update spam, this is the last one: remove unused lines from test.

Also /s/var/list/ in the test filenames.

Addressing the review comments: remove the note-to-self comments, and rename DBG_VALUE_VAR -> DBG_VALUE_LIST. This could change again if people think something else would be more appropriate, but this looks to have general approval.

I think the preferred way of resolving this would be to post to llvm-dev and make sure the authors of D70642 are CC'ed. As far as I can see D70642 is less space-efficient, but allows for referring to the same argument multiple times. Given the DW_OP_dup/over operators, I'm not sure if that is necessary.

In D82363#2109399, @probinson wrote:

DBG_VALUE_LIST? And the predicates could be isDebugValueList and isSimpleDebugValue or something along those lines.

In D82363#2108817, @djtodoro wrote:

Is there an overlap of the DW_OP_LLVM_arg with the D70642?

Allow for malformed DBG_VALUEs to be printed.

Fixes broken tests and code error.

Closed, since as per the discussion above this change is undesired and unnecessary.

In D75337#1901253, @StephenTozer wrote:

Actually, after looking at how this is being used in the child revisions, it looks like this should be folded into handle_options

Actually, after looking at how this is being used in the child revisions, it looks like this should be folded into handle_options - that function is already used to check whether the supplied options to a tool are valid, and raises an Error if not. That seems more consistent with the rest of Dexter than returning a ReturnCode.FAIL, which is currently only used if the test is run without any errors but has a failing score.

LGTM except for the inline comment.

LGTM

In rG61b35e4111160fe834a00c33d040e01150b576ac#890707, @akhuang wrote:

Ah, I see. The file I used to reproduce is attached on this bug. I think it's being called from SimplifyCFG.

In rG61b35e4111160fe834a00c33d040e01150b576ac#890518, @akhuang wrote:

This is causing a timeout in chromium code. I haven't yet gotten a reduced repro, but it looks like there's an infinite loop.

In rG61b35e4111160fe834a00c33d040e01150b576ac#890518, @akhuang wrote:

This is causing a timeout in chromium code. I haven't yet gotten a reduced repro, but it looks like there's an infinite loop.

Fix spelling error.

Add test, address nit comment. Note that some of the current test checks should pass even without this patch if commit 636c93 is not reverted, as some of the redundant debug info would be dropped.

Merged in commit: llvmorg-10-init-12272-g89d19d60adb9

Format, addressing review comments

@djtodoro Does this latest change look good to you?

Use shared functionality for zero/sign extension ops, fix tests

Rebased and updated for latest master, fixed clang version and unused attributes in tests.

Use Undef int1 when undef-ing a DbgVariable without a type.

Maintain order of debug intrinsics in empty block elimination, fix test

Merged in commit: llvmorg-10-init-11043-g269a9afe25cb