Code changes LGTM pending inline comments. I haven't looked at the tests yet.

LGTM (pending clang-formatting if not already, plus a few nits).

Thanks @scott.linder

(forgot to hit Accept)

LGTM, including agreeing with your assessment of and changes to llvm/test/CodeGen/AMDGPU/llvm.dbg.value.ll (comments from @arsenm or @scott.linder to confirm would still be welcome of course, if either of you have the bandwidth)

LGTM (with formatting nits)

LGTM

There are a load of getFirstNonPHI() -> getFirstInsertionPt() that I think should be getFirstNonPHI() -> getFirstNonPHIIt() (marked inlined) based on the discussion in D152468, otherwise LGTM.

Still LGTM

LGTM

LGTM (with a few nits)

LGTM too with some inline nits/questions.

The result of this change is that debug locations are preserved after sinking. I think this is a situation where the docs specify that we should not preserve the source location of an instruction. Here is an excerpt from the linked doc explaining when we should preserve debug info:

I started reviewing this but didn't have time to finish it. For fear of losing my comments I'll post what I've got so far. Note to self: start at Instruction.h.

Just a couple of inline nits/questions from me.

The mechanical change itself LGTM.

The changes from getFirstNonPHI to getFirstInsertionPt make me worry that this isn't NFC as it looks like the latter skips past EH instructions.

In D153950#4455311, @StephenTozer wrote:

The pass inserts a PHI, so needs to reset the NoPHIs property - makes sense. The property reset could be taken out of the loop and put under a conditional if (MIItBegin != MIItEnd) - I'd leave that as optional though, because I imagine it makes very little difference in practice.

The verifier issue should be fixed in D153950. Slightly surprised that this test is the first to find the issue... grepping for x86-cmov-conversion in llvm/test only shows this new test.

Looks like this is causing a crash on current main: https://github.com/llvm/llvm-project/issues/62838

+ Added more CHECK lines to the test

ping

+ address review comments

In D146987#4375881, @hctim wrote:

Hey, found another error that occurs when building Android that looks to be different from @MaskRay's revert.
...

That's reasonable, and option 1 still looks good to me given what we've discussed, so LGTM. Thanks!

Great, thanks for the review.

In D151326#4368227, @jryans wrote:

The llvm.org/PR123-style link in the patch summary doesn't seem to work. Perhaps for GitHub-only issues you need to use a direct link like https://github.com/llvm/llvm-project/issues/62838?

It might be worth posting your question on discourse in the LLDB subcategory for greater visibility.

Thanks. Yep they are dropped, causing locations terminated by the dbg.value to no longer be terminated; incorrectly extending coverage of the preceding variable location. (EDIT: That's the behaviour without this patch, to be clear).

In D150577#4345930, @bjope wrote:

In D150577#4345584, @Orlando wrote:

Hi @bjope

Is there a reason that findDebugLoc and rfindDebugLoc call skipDebugInstructionsForward / skipDebugInstructionsBackward rather than next_nodbg / prev_nodbg. It looks like calling the nodbg functions could solve the problem (the end-iterator-dereference) too, with no new code, but I am not certain of that without testing it.

The findDebugLoc and rfindDebugLoc functions are taking the DebugLoc from the MI pointed to by the iterator, if that MI fulfil the requirements. So they kind of do "skip-debug + examine MI + step-iterator", while the findPrev variants do "step-iterator + skip-debug + examine MI". The next_nodbg / prev_nodbg is a short form for doing "step + skip", so in order to use them one would need to rewrite the code a bit more.

It had perhaps been more interesting to have helpers for scanning forward/backward, and doing pre/post increment. That would give four different variants. Now we got two variants that scan forward doing post-increment. And two variants that scan backwards doing pre-increment. I got a bit confused about that for awhile (and it is not that easy to understand what prev/next, first/last, forward/backward etc means when having variants using both iterators and reverse iterators). One idea with the unittests I added is to actuallly show what happens when using those helpers (even if one still would need to add some debug instructions to make those tests even more complete).

If you're testing the API you might need -DLLDB_ENABLE_PYTHON=On too which enables the LLDB python bindings. That option is present in the Cmake step of the logs. (Not an LLDB dev, just passing by).

As I understand it, this change means the behaviour of rfindDebugLoc when given instr_rend() is now similar to the existing behaviour of findPrevDebugLoc when given instr_end(). findPrevDebugLoc calls prev_nodbg (code) which decrements the iterator before calling skipDebugInstructionsBackward. I.e. In both cases an end iterator is valid input and will not be dereferenced. (SGTM)

That should be fixed with D149959

Thanks for the reproducer and report! I'll have a patch up for this shortly.

Nice, that's much better!

I've just landed this again.

Thanks!

This seems sensible and the test looks good. I've added other reviewers who might want to take a look.

Abandoning this (see previous comment)

Nice catch!

+ Address review comment

In D146987#4299081, @paulkirth wrote:

@jmorse Sorry it took me a bit to set up, but I ran an experiment in our CI that reverted the change that disabled this as default and tried it for x86. It looks like this patch is working OK for us now: https://ci.chromium.org/raw/build/logs.chromium.org/fuchsia/led/paulkirth_google.com/df38ff794f44284ff34f63e5dc1f1d41b25225b1241c14eff15a9f0a4b189afb/+/build.proto?server=chromium-swarm.appspot.com

I'm reasonably confident that should mean you won't run into issues. The caveat here is that I can't easily launch experiments across all combinations of platforms and hardware, so it's possible there may still be an issue. I think that's unlikely given how this has been manifesting, so I'd say you're good to try again in our perspective.

Most curious.... I'm not sure off the top of my head why that'd be the case (either the issue or the fact that D140901 fixies it). SelectionDAG has a load of weird edge cases with debug info handling though. Thanks for looking into this, is there anything I can do to help?

Thanks

Thanks everyone for reviewing this (and the rest of the stack), sorry it has taken a while to get these landed!

This patch prints the tuple as !{} even if it's distinct, which I thought was reasonable when writing the patch. If a distinct empty tuple is used multiple times or is attached to multiple instructions however it might be useful to have the metadata ID printed, to make debugging easier.

Thanks for the heads up @thakis. Looks like there are simply some more tests that need updating to the new formatting, but I've reverted for now as it tripped several bots.