This sounds similar to PR41675 -- sometimes the DWARF expression builder interprets a DW_OP_deref as an extra dereference, other times it interprets it as confirmation that the expression being built is a memory location.

LGTM, thanks for the patch!

Probably, it is OK if dbg.value was not originally _supposed_ to be a dbg.declare but was converted in this place.

Hmm, it looks like the extra DBG_VALUEs dropped in the msp430 test were legitimate locations for "b" -- if getUnderlyingArgRegs now fails but generates multiple {0,0} entries, the ArgRegsAndSizes.size() > 1 block on line 5487 generates no (well, zero sized) locations and returns true. Wheras before EmitFuncArgumentDbgValue returned false, and another code path found a location for "b".

I got curious; the scenario I described happens in test/DebugInfo/MSP430/sdagsplit-1.ll, where one of the operands to a pair is a load.

This looks good to me, and I'm sort of familiar enough with the debug bits to approve. I've one concern about the change to getUnderlyingArgRegs though: previously it would fail-safe and return 0/$noreg if it encountered a node it didn't recognise. However now, if there's anything unrecognised in the middle of a vector/pair, the element will be silently skipped. That could lead to splitMultiRegDbgValue producing DBG_VALUEs with the wrong fragment offset, as it's unaware of the skipped element.

This is a great bug find -- I'd no idea SROA ran twice! Applying this patch gives a ~3% increase in variable locations covered on a clang-3.4 build.

! In D58453#1582991, @yurydelendik wrote:
Looks like I misunderstood meaning of *all*. I assumed that means DBG_VALUEs the refer the vreg def of the source instruction, excluding DBG_VALUEs that refer the same vreg def by the above instructions (in the same block). The test result above shows that collectDebugValues locates all DBG_VALUEs for the instruction even if it does not (re)define the value, but there are few instructions in the same block above that do. Is this the intended effect of this patch? If yes, we probably need to create more robust collectDebugValues-like method that will track lifetime of DBG_VALUEs.

Happily D58453 killing off a large amount of placeDbgValues activity significantly reduces DBG_VALUE grouping -- I don't have the numbers to hand, but I would say the density was almost an order of magnitude lower. The largest back in the benchmark I referred to was about ~120, and other large packs occurred much less frequently.

I've added some changes to the patch that makes collectDebugValues behave in the way I described, returning all debug users of a VReg, optionally limited to only the basic block of the source MachineInst. This should be useful for testing; and it's immediately caused the test/DebugInfo/WebAssembly/dbg-value-move-reg-stackify.mir [0] test to fail. The output is:

Refresh this patch:

• Rebase it onto the current open stack
• Remove a bunch of needless attributes and metadata

Split up subregister comparision into pairs, to make it clear we're comparing unsigneds.

Facepalm: it looks like I mucked up the diff before uploading, the patch originally came with this change to MachineSink, not just the test. Oooff.

In D58191#1579194, @vsk wrote:

This test looks reasonable. Is D58238 the only MachineSink change you have queued up?

Archaeology-ping: this patch avoids super/sub-register sinking leading to DBG_VALUEs referring to the wrong location, would be good for LLVM-9.

In D58453#1564358, @yurydelendik wrote:

The change will be OK: we rely on collectDebugValues in WebAssembly to locate instruction's debug values. And we worry only about these atm.

Rebase, update jump-insts in MIR test to latest version, address review comments (cheers).

Vedant wrote:

This looks reasonable to me. Mind adding test coverage for simple tag_offset/fragment expressions?

Ancient-ping on this, with the aim of it helping the placeDbgValues patch in. To re-summarise: this collects DBG_VALUEs to rewrite while walking through MachineCopyPropagation, rather than relying on the all-dbg-values-follow-the-defining-inst assumption.

Abandoning this WIP -- addressing the "values are in two places in SelectionDAG" problem isn't going to be so easy to solve.

Bjorn wrote:

My feeling is that the cases where we now lose some debug info is quite rare (at least for our DSP-C/DSP target), and the benefit of making the register coalescers handling of DBG_VALUE more sound outweigh that problem.

In D56151#1558161, @aprantl wrote:

I'm reading this as: "this patch make debug info more accurate by rejecting invalid DBG_VALUEs earlier on, but it overshoots the target a bit and also rejects a few(?) false positives". I support this approach, as we all want to move LLVM into the "more accurate" direction, assuming that the false positives are fixable and the ratio of rejected incorrect DBG_VALUEs to false positives is favorable.

Ping: we should hammer out this review, seeing how the branch date for llvm-9 has been announced. Could I suggest that everyone is happy with the *implementation* of this patch, but that it's not yet agreed that it needs merging?

Freshen up patch to make it merge cleanly; check that DVI isn't null... not clear whether something has changed or if I uploaded a completely broken patch before.

Account for DW_OP_LLVM_tag_offset when working out whether a DIExpression is "complex" or not.

Add an isComplex() method to DIExpression that detects when an expression could be interpreted as a memory location.

Semi-ping: while this is approved, the extra interest suggested I should delay committing in case there's more discussion. Without hearing anything else, I'll land this in the next couple of days.

Sort-of ping at @bjope on this: this is fundamentally a speculative patch as I can't dig into the regressions being seen in D56151, and I have no expectations on other peoples time; however we'll need to think about whether those regressions block the placeDbgValues chain-of-things going into llvm-9, which will branch sometime soon (next month?). Similar to what I wrote in D56151, we have a choice of poisons for llvm-9:

• Leave placeDbgValues as it is,
• Disable placeDbgValues but don't commit D56151, which may cause dead variable locations to be resurrected during register coalescing,
• Disable placeDbgValues, commit D56151, triggering the regressions @bjope saw there, which may or may not be alleviated by this patch.

Address feedback / review

Follow up in https://reviews.llvm.org/rL363259

Blast, missed the final nit, will patch that in a second.

Use getValueOr when extracting the value of a fragment with a default; assign some better variable names to container references in accumulateFragmentMap

Switch instances of "frag" to "fragment" -- this only forces more wrapping on a few lines and is well worth it. I've also scattered a few more initializer-list replacements for make_pair, and added a fragmentsOverlap helper to DIExpression.

In D62904#1537110, @aprantl wrote:

Please note that SROA is not the only producer of DW_OP_LLVM_fragments. The Swift frontend, for example, can produce fragments from the point where LLVM IR is generated long before LLVM's SROA is invoked. If a condition is not enforced by the Verifier it is not generally safe to rely on it because LLVM IR is generated by many producers, not just Clang.

Add a "default" fragment for DBG_VALUEs that don't have one, which covers all possible fragments. This causes non-fragment DBG_VALUEs to overlap all fragmented DBG_VALUEs.

In D63083#1536776, @rnk wrote:

I should add, it's really just the commit message that I find confusing. I think the documentation as written clearly documents LLVM's existing behavior.

Whitespace change to ease reading of .rst, split summary into being an actual commit message

Superceeded by D62904, I felt it cleaner to upload a new review req.

Ninja-edit: improve the comment at the top of the added regression test.

LGTM -- this is the already-accepted patch with trivial change, the real difference is that a bug got fixed elsewhere.

Looking at the delta between the approved and ~final version, it looks good to me, seeing how the bug this tripped over was fixed in D61933 and nothing in the code has changed.

Ping on this -- AFAIUI people believe the change is good, and depending on FrameDestroy is deployed in a separate patch.

LGTM with a couple of questions, will leave it for further comments.

Move the change in a comment into this patch; remove an un-necessary test comment.

Bjorn wrote:

Is the intention to change the behavior for prologues as well here? The description mentions that we do a different analysis for the epilogues. But with this patch we no longer ignore register clobbering for "FrameSetup" (collectChangingRegs used to ignore FrameSetup code).

Maybe that is a good thing (just not obvious when reading the description that this patch also treats prologues differently).

With this update I've separated the FrameDestroy portion from the rest of the patch. The result is that debug-loc-offset.mir and pr19307.mir are marked XFAIL, as the locations they check no longer extend through the frame-destroy'd part of the produced code. I'll upload the remaining part of the patch in a moment, that adds in use of the frame-destroy flag and un-xfails those tests.

This looks good to me (I'll leave it for more comments).

In D61600#1509763, @NikolaPrica wrote:

I suppose that this is ready to land?

Apply review comments (change a vector size, shuffle the order of some blocks).

Nikola mentioned in D61890 that the changes to fission-ranges.ll result in reduced variable location ranges, which is unfortunate. IMO, eyeballing the test, I think the input IR we have at the moment is wrong, and it was only the bug that this patch fixes that caused us to get the output right.

The reason I offered this patch as a alternative to D61600 is that it degrades some ranges when it could leave them wider (test/DebugInfo/X86/fission-ranges.ll).

LGTM (I think I can approve?). The source IR needing no-frame-pointer-elim seems like enough reason to keep that attribute around.

Update diff to more production-ised version, will edit summary and add reviewers shortly

Just to note: the patch this is based on (r359426) was reverted in r360301 due to compile time issues. I've got a patch incoming (tomorrow) that relies on this patch to fix it: they'll need to be un-reverted / committed in a certain order and possibly all together. (We can work through that when this is approved though).

Hmmm, I get what the patch is doing, but I have the feeling that it might be the wrong place (or impossible) to fix in LiveDebugValues. Given the sample source in dbg-live-debug-values-end-range.mir, would I be right in thinking that this patch is to fix the ChangingRegs behaviour that David brought up in [0]?