User Details
- User Since
- Mar 28 2019, 8:58 AM (235 w, 1 d)
Fri, Sep 22
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).
Tue, Sep 12
Thanks @scott.linder
Mon, Sep 11
(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)
Fri, Sep 8
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
Tue, Sep 5
LGTM
LGTM (with a few nits)
Fri, Sep 1
Aug 22 2023
LGTM too with some inline nits/questions.
Aug 7 2023
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:
Jul 28 2023
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.
Jul 24 2023
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.
Jun 28 2023
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.
Jun 27 2023
Jun 13 2023
Looks like this is causing a crash on current main: https://github.com/llvm/llvm-project/issues/62838
Jun 12 2023
Jun 9 2023
+ Added more CHECK lines to the test
Jun 8 2023
ping
Jun 1 2023
+ address review comments
May 31 2023
May 24 2023
That's reasonable, and option 1 still looks good to me given what we've discussed, so LGTM. Thanks!
Great, thanks for the review.
May 18 2023
May 17 2023
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).
May 16 2023
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)
May 5 2023
That should be fixed with D149959
Thanks for the reproducer and report! I'll have a patch up for this shortly.
May 3 2023
Nice, that's much better!
Apr 28 2023
I've just landed this again.
Thanks!
Apr 27 2023
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
Apr 26 2023
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!
Apr 25 2023
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.