Paging @vsk -- it was a while ago, but do you still have the bitcode from D7406 handy, and would you have spare cycles to test it against this (combined patch in D94631)?

Remove needless redirect of subprogram creation; add original compile commands to test case.

Responding to inline comment that's now sort-of-orphaned: calling back out to constructSubprogramDefinitionDIE sounds good, I've added that to DwarfUnit::getOrCreateContextDIE with a comment.

Change interception of Subprogram creation to call back out to DwarfDebug instead of looking up CUs within DwarfUnit.

Replace regression test with David's smaller test

LGTM with comments inline. While it's a simple fix, I'm not especially familiar with coroutines, so it'd be nice if someone else could chime in before landing.

This improvement looks good and preserves more source locations. Awkwardly, as @vsk says it's quite different from the other documented rules, because it relies on the context of where the instructions are going to be placed.

I wrote:

(Nabeel and I share a corporate overloard, we should leave this a little more time in case anyone else wants to chip in).

LGTM, although couldn't you also just swap the basictest.ll DEBUGIFY-NEXT to be a DEBUGIFY-NOT: call void @llvm.dbg.value and not add the extra test.

LGTM

LGTM

Could you upload the patch with context please, as it's easier to navigate the changes. Instructions can be found here: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

'Tis the season to ping!

I guess that is possible. As long as both frame objects have the same Stack-ID, StackSlotColouring can merge them together. However, in that case, the sizes and offsets of those objects would both be scalable and comparing these sizes would be no different as they would be for non-scalable sizes.

Ah, I hadn't quite lined up in my head that the offset scales as the stack grows downwards. With that in mind, I think this sounds safe -- the scalable part of the stack offset is opaque as far as LiveDebugValues is concerned, and we can just pass it around.

LGTM, this sounds like it works just as well as before, thanks for pursuing.

Forgive my ignorance on scalable vectors; but is there definitely a scenario where the "Scalable" portion of the stack offset is nonzero, and if so could the test cover it please.

(Responding to inline comments) I think the main concern with hard-coding registers in DIExpressions at this stage is that LLVM isn't able to identify when that register is clobbered later on, and terminate the variable location. However, as you say:

I'd like to chime in and point out a slightly related scenario [0] where we have to interpret a DIExpression to work out how adding operators is going to change the meaning of the expression. I believe in the long run we need metadata describing "the attributes of this variable location" rather than packing information into DIExpressions and having to interpret it.

A few more comments inline. I think everything in this patch works; there's still an open question in my mind of why we need to create the implicit variables so early, during instruction selection. Isn't it sufficient to leave a:

FTR, this LGTM

LGTM too

@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 tried running this patch on my machine though and the tests always pass (using llvm-lit) even if I change numbers, so I don't know if something's going on with the directory test format?

LGTM; the point of the "Emulate VarLocBasedLDV" flag was to help prove that the overall algorithm matched what was already produced by LiveDebugValues. Seeing how the approach seems to be (broadly?) accepted / acceptable, it's to just ditch the flag altogether, it doesn't serve any purpose now IMO.

Hi Bruno,

Thanks for the catch!

bruno wrote:

That's a good question, I initially thought a dbg.declare dominating the BB in question would be enough, perhaps the debugging experts could help clarify what's going on.

LGTM, anything and everything in this vein much appreciated.

(Rebase due to me fiddling with the signature of substituteDebugValuesForInst on master).

LGTM; thanks for diving in!

(rebase again to see if phab will accept more information about base commits and the like)

Sorry it took so long to get back to this,

(Rebase now that I've dropped some "REQUIRES: X86" lines in the modified tests...)

Rebase to account for me gluing patch series together; one or two new code paths crop up where one has to decide whether to try and use an entry value OR register a debug use-before-def.

Rebase onto existing patches to make this applicable through arc; one line marked "future work" in livedebugvalues_instrref_tolocs.mir now has a variable location, as this is the future work that implements it.

(Update for a conflict with current master)

NB: I said in the test:

LGTM, I agree the subregister index on any undef DBG_VALUE is meaningless.

(Revise to address comments)

In D89088#2342922, @jmorse wrote:

If an entry block with a predecessor is legal behaviour [...]

In D89088#2342853, @dmajor wrote:

Adding @jmorse in case D83046 is related.

(Adding debug-info),

I've reverted this and the follow-up patch in rG0a7f41739fd994a50 as it continued to fail tools/llvm-cov/warnings.h on Windows-based platforms.

Updated patch, changes signature of substituteDebugValuesForInst to take a "number of operands to look at" argument, defaults to "all of them".

After landing this, bkramer pointed out one of the assertions in substituteDebugValuesForInst was ineffective. Fixing that shows that some callers (see patch 5 in this series) do so with different instruction signatures (i.e., different types and different numbers of operands).

Hrrmmm, that smells fishy; thanks for the warning sqelching, I'll follow it up.

The variable location changes LGTM, I otherwise defer to Djordje who's made a request.

ping; (also remove from summary the reference to additional tests needed, as these are now in the patch).

This completely fell off my radar sorry,

Assuming I understand the history correctly, this patch in itself won't introduce any inaccuracy into debug-info, and has a motivating performance example, so LGTM.

This fixes things for us, and by limiting to one optimisation pass it's much less brittle for the future -- much appreciated!

In D88363#2317241, @Mordante wrote:

Can you explain the kind of issues you're having?