User Details
- User Since
- Mar 28 2019, 8:58 AM (170 w, 6 d)
Today
LGTM with some minor comments. I'd feel more confident if someone else (/ @jmorse ) took a quick look too, but given this is NFC and conceptually straightforward, I'm happy to approve it.
Mon, Jun 20
LGTM
Thu, Jun 9
Wed, Jun 8
The code changes look good to me, so just waiting for the test before approving now.
Jun 7 2022
Should add a .mir test
I've added an inline suggestion but I'm happy with or without that change, LGTM.
LGTM
LGTM!
Hi @jryans, thanks for this update. I'm not particularly familiar with reStructuredText formatting so I've just been looking at an existing doc (https://github.com/llvm/llvm-project/blob/main/llvm/docs/LangRef.rst) to guide my review.
May 23 2022
Just to confirm, does this comment:
Remove unused "zero range" test.
It looks like the clang front end emits a size field for reference types. e.g.
May 17 2022
LGTM with some nits addressed, thanks! I've hit accepted but please leave this up for a day or so in case anyone else has any further comments.
May 9 2022
The latest change makes sense, LGTM
Apr 29 2022
Makes sense, LGTM. I've added two tiny suggestions inline but I don't mind if you leave the patch as it is.
Apr 26 2022
LGTM. It's great to have the comprehensive test spelling out the expectations for all the combinations of fragment / deref / indirect.
Apr 22 2022
LGTM
Apr 20 2022
Apr 13 2022
Apr 4 2022
I wonder if the test IR could be simplified. Would it not be enough to have a function with a single dbg.value and nothing else?
Mar 17 2022
FWIW the change LGTM mechanically and I think it sounds reasonable (/good).
Mar 15 2022
Hi @yln, the test you added llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll is failing on some bots, e.g. https://lab.llvm.org/buildbot/#/builders/139/builds/18527 (builder llvm-clang-x86_64-sie-ubuntu-fast). Please can you take a look?
Mar 10 2022
Makes sense to restore NFC-ness of the patch, LGTM with that change.
Mar 9 2022
Please wait for the other reviewers to be happy, but this LGTM with some nits addressed.
Mar 8 2022
Mar 7 2022
The change SGTM but I found a reproducer that causes clang to crash when this patch is applied:
Hi, the test add-remove-irrelevant-module-map.m starts failing with this commit on the llvm-clang-x86_64-sie-win builder: https://lab.llvm.org/buildbot/#/builders/216/builds/929. Please can you take a look when you get the chance?
Mar 4 2022
I can clarify the comment. It's my understanding that non-empty DIExpressions must have a DW_OP_stack_value terminator. The expressions generated by the SCEVDbgValueBuilder objects do not have these terminators (as the expressions may be inserted into other expressions). If the pre-LSR expression was empty, once the complete salvaging expression has been generated, then the terminator must be appended.
This should improve the readability of the output nicely. LGTM.
LGTM
Mar 2 2022
I don't think there are "rules" about tagging patches as NFC (just put NFC somewhere in the title). It's definitely helpful to be able to see at a glance though - two examples: if an NFC patch is causing changes in output there's a bug, and if a patch is NFC it probably doesn't need any tests.
Feb 28 2022
LGTM
Feb 25 2022
Can this be labelled NFC or is there a functional change here that I've missed?
Thanks @gkistanova. I don't think I have commit access to llvm-zorg. Please could you land this for me when you get a spare minute?
Feb 18 2022
Feb 17 2022
Feb 11 2022
LGTM with query; More restructuring to avoid more indentation would be nice, but here we are ._.
+ addressed inline comment
Feb 10 2022
Feb 9 2022
+ Improve comment in test
+ Fix description
+ Rebase (fix conflicts with D118468)
Feb 8 2022
LGTM. Converting badly-formed variable locations into "no location" rather than crashing the compiler SGTM. Is it worth fixing the defect in the bug report too in a separate patch?
Feb 7 2022
+ Make gdb-version-check more robust by checking version string length
+ Catch-em-all when trying to run gdb
+ Print a message when the XFAIL condition is detected.
+ Fix stderr -> sys.stderr in a print call.
We've now got a native Linux builder in staging where you can see these tests failing: https://lab.llvm.org/staging/#/builders/205/builds/91
We've now got a native Linux builder in staging where you can see these tests failing: https://lab.llvm.org/staging/#/builders/205/builds/91
Feb 3 2022
Feb 2 2022
Nice. I have a few questions / nits but I'm a fan of the change.
Thanks for taking a look @probinson. Adding Jeremy as reviewer.
+ llvm-commits
Feb 1 2022
LGTM
Jan 31 2022
LGTM
Nice! I have one question inline but this SGTM.
Once @dblaikie's concerns are addressed, this LGTM.
The clang-format bot isn't happy and I have one nit inline, otherwise LGTM.
Jan 28 2022
nit in commit message:
This patch releases some memory from InstrRefBasedLDV earlier that it would otherwise. The underlying problem is:
"earlier that it would" -> "earlier than it would"
Jan 26 2022
Folded nit-fixes into the commit, thanks both.
Jan 24 2022
Jan 21 2022
LGTM
Jan 18 2022
That clears up a lot of my confusion.
Jan 17 2022
Sorry it's taken me a while to get back to you on this - thanks for all that info. Here is a summary of the situation as far as I understand it:
Jan 13 2022
+ Fix comment in test: -g -> -gdwarf-5
Thanks for taking a look @probinson.
Jan 12 2022
Jan 10 2022
(Still LGTM)
Jan 6 2022
Hrm :/ I'm not super enthusiastic about either direction. I'm starting to wonder whether this general direction is viable - that some DWARF consumers might not find types that are in type units but aren't referenced from the CUs at all.
Do you have a measure of the debug info size improvement of this direction? If it doesn't end up counting for much, maybe it's not worth trying to create this possibly novel debug info?
Jan 5 2022
Please can you update Commands.md too? LGTM with nit, thanks.
Jan 4 2022
Thanks very much for the reproducer and revert @dblaikie.
Dec 17 2021
There's a mistake in the commit message, I meant "Example of failing bot: https://lab.llvm.org/buildbot/#/builders/123/builds/7772".
I appear to have broken a bot here https://lab.llvm.org/buildbot/#/builders/105/builds/18850, looking now.