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?

Update comments in placeDbgValues to reflect its new purpose; remove code calculating the previous non-debug-instr, as we now use a dominator tree check instead.

Use isCopyInstr to detect copy instructions, which will catch more opportunities that just isCopy().

This problem had PR40427 hiding at the bottom of it, so this WIP isn't required.

It turns out the validity of this change relies on placeDbgValues reordering (curses), a small amount of extra juggling will be required.

In D58238#1399186, @bjope wrote:

I have some doubt about this. Mainly the part about inserting undefs. Although, some of my doubts are partially based on the big-black-hole regarding how debug info is supposed to work for heavily optimized code (and more specifically how this is supposed to work in LLVM based on what we got today).

Explicitly test for whether we're pre or post regalloc when deciding whether a copy can be propagated, refactor test logic.

Sort dbg-values to be sunk to ensure determinism, sprinkle -NEXT on some CHECKS to strengthen test.

The filtering from r353735 happily makes the sparc test change un-necessary.

Rebase onto Bjorns changes, remove now un-necessary test change.

Also, thanks for reviews!

(Plus I added a new test to cover this behaviour, and removed some plumbing for salvageDebugUsersForDbgValues that was folded into an earlier commit).

The issue with generating new DW_OP_derefs has been resolved by r353824, as a result we no longer change existing tests with this patch.

Ping, @CarlosAlbertoEnciso how's the utility function?

Folded last comments into commit, but phab doesn't show those comments inline because of path differences it seems:

• Added comment warning against salvaging loads in salvageDebugInfoImpl
• Removed TBBA metadata and un-necessary DILocations
• New test case positively checks for an undef dbg.value corresponding to the optimised-out load.

In D57702#1391414, @aprantl wrote:

plus it'd be good to hear Adrians view on the fragment scenario.

Do you have any specific questions in mind?

In D57962#1391428, @aprantl wrote:

Would it be feasible to inject a dbg_value(undef) at the end of the basic block or before the next store, whichever is earlier, instead?

This turned out to have PR40427 burried at the bottom of it, and was fixed in r352350.