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]?

For some reason I hadn't clocked that the IRBuilder deals with blocks in a state of partial construction/correctness (duh), and had assumed there would always be a terminator in each block, whoops.

Add comment explaining the choice of zero line numbers in getDebugValueLoc. After discussion in D61198, I've removed the clause preserving the line numbers of store instructions -- It's probably better to make it clear that the line number information in debug intrinsics is completely meaningless, and thus not try to preserve it at all. The selection of a zero line number is thus just a defence against leaky optimisations.

Remove an accidental isIndirect=true flag, my bad.

Scatter comments / address review

Add the test change I forgot to stage (whoops) -- the AMDGPU test here doesn't have any non-meta instructions with line numbers. Prior to this patch the debug intrinsic line numbers leak in, which the test depends on. Fix this by giving the real instructions line numbers. (The test is for placement of dbg.values rather than line numbers anyway).

Ping; I've uploaded D61198 which likely eliminates the lions share of DebugLocs-leaking-from-DbgIntrinsics. This patch would still be required however to reduce the damage from other scenarios where DebugLocs leak from debug intrinsics.

Hi, I've uploaded D61181 with a patch that might fix those regressions -- if you happen to have any spare cycles for evaluating it that'd be appreciated.

Rename variable, improve comment, sprinkle a little clang-format.

I've rebased this patch to be based on r358073, Davids improvements to DbgEntityHistoryCalculator. Rather than tracking non-register DBG_VALUEs separately, this patch now iterates over all open DBG_VALUE ranges and either terminates the constant ones, or clobbers the register ones. This should be the same behaviour as the previous patch.

NB: I've written up a sort of broad-ish summary of what's wrong with the SelectionDAG scheduling of DBG_VALUEs in PR41583 [0], which IMO is the cause of the unfortunate variable-location droppage described above. I've got an additional patch that should go up tomorrow.

Thanks for the all the reviews; still outstanding is the dbg.declares-get-stashed-in-MF-objects matter, which I'll generate a follow-up for.

(I'll commit)

Many thanks for the reviews, I've juggled the text in the first third in response to comments,

Maybe ISel should have put the COPY to %0 after the last DBG_VALUE (now we got a dbg-use of %15 after the last non-dbg-use of %15). Or maybe it should have used %0 instead of %15 in that DBG_VALUE. Or maybe there should be one DBG_VALUE before the COPY using %15 and one after using %0.

LGTM, many thanks for the find and fix!

Ping on this -- I think we all agree that this is a part of a large issue (DBG_VALUEs of non-live variables), but this patch is a step in the right direction.

In D57694#1441530, @srhines wrote:

Switching it to a MapVector makes it deterministic. I was just struggling late last night to try to reduce the .ll file down to something worth checking in! If you want to take over to minimize the test case, that would be much appreciated. Let me know so I can continue doing that if needed. Thanks.

In D59272#1439778, @bjope wrote:

Isn't there a risk that this (partially) is hiding bugs in some other passes, where the DebugLoc is picked from the wrong place. I guess that either it should be picked from an adjacent non-meta instruction. Or it should be set to unknown.
With this patch we only eliminate the risk that the faulty passes are picking an "incorrect" DebugLoc from a dbg.value intrinsic (or DBG_VALUE instruction). But the "faulty" pass could still pick incorrect DebugLoc from other instructions, right? Or is it always correct to take the DebugLoc from all other non-dbg.value instructions, including all other kinds of meta-instructions?

Might be hard to find all such bugs. But with your known examples it shouldn't be too hard to find at least some places in opt/llc where the DebugLoc from the dbg.value is infecting some other instruction. Then perhaps we want to land this patch as well, just do avoid some not-yet-detected problems.

In D57694#1440571, @srhines wrote:

This patch appears to generate non-reproducible builds in some cases. I can craft a more minimal test case, but the following link (https://godbolt.org/z/sWucUZ) is what I have been using. If I run Clang multiple times, the output eventually swaps the order of some undef DEBUG_VALUE's. I am just passing "-O1 -g" with that .ii file. It isn't obvious to me yet what is causing this to be unordered/non-deterministic.

In D58453#1441146, @jmorse wrote:

this could land right now.

In D58453#1439722, @bjope wrote:

What is the status regarding this patch? Are we waiting for something?

In D59431#1436497, @aprantl wrote:

So I guess the answer to my question whether this is indicative of a bug in an earlier pass is: not necessarily.

In D59431#1431410, @aprantl wrote:

Isn't that a symptom of previous passes failing to insert a dbg.value(undef) when they deleted a dbg.value? I'm trying to figure out what a legitimate example for this would look like.

Update to use unknown-locations for everything but promoted store instructions,

Many thanks for the reviews!

Ping -- AFAIUI we're happy with the meaning in this patch, wording and presentation still needs review?

In D58238#1422261, @bjope wrote:

Yes, I was just thinking out loud about sinking/scheduling in general and not so much the specific situation with MachineSink. But I also wondered if the sinking done by MachineSink could be seen as a special case of sinking in general, where we sink one instruction at a time. For MachineSink we eventually reach the end of the BB, and then continue into a successor. So how do we determine when it is time to insert "undef" when sinking one instruction at a time? What kind of reorderings should/shouldn't trigger that we insert an "undef"? I guess this is one thing we should try to describe in the documentation (also for DGB_VALUE), to make sure new developers understand the basic logic behind how we implement these things.

I produced another patch (D59027, work-in-progress) that only creates undef DBG_VALUEs when the order of assignments would change... however I then realised I'd misunderstood Bjorns question here:

Re-think opening of paragraph on the risks involved when moving/altering code.

Incorporate further wording and structure feedback

Incorporate recommendations on using auto from Andrea, delivered offline,

LGTM, although I'll wait a bit for more opinions

Incorporate feedback: use skipDebugInstructionsForward for better readability, iterate forwards when sinking DBG_VALUEs.

Rename new test case's file name, move it to the 'DebugInfo' directory too as that seems more appropriate.

Avoid relying on placeDbgValues for this change. We already walk (forwards) through all instructions in a block looking to optimise them, add a dbg.value visitor that rewrites the dbg.value operand if it refers to a sunk address computation.

Sprinkle some possessive apostrophes, speling, incorrect variable names

Apply review feedback, re-write the dbg.value undef example to better demonstrate the problem at hand.

I've generated a short summary in https://reviews.llvm.org/D58726 which illustrates how I believe dbg.values are to be interpreted -- I'll chuck this at llvm-dev tomorrow if it's not obviously broken. As mentioned in the summary, it's likely that more could be said, but getting agreement on the location of a dbg.value being the location that an assignment ``happens'' would be good.

Hi Paul,

In D58238#1406919, @bjope wrote:

So then we should check if there is any other dbg.value that we sink past? Otherwise there won't be any reordering and no need for an undef location?

In D58453#1406020, @probinson wrote:

In my view, the real problem as stated in the PR appears to be that the value later gets *lost*, sadly before the instructions directly related to the switch-case where the variable is used; the real problem is *not* that the value's description starts too soon.

So given all that, how does this patch help address the real problem?