- User Since
- Mar 19 2018, 2:57 AM (168 w, 5 d)
Thu, Jun 10
Fri, Jun 4
Thu, Jun 3
Ping and refresh of this patch; I've replaced a multimap with a sorted vector, used a class to store information about seen DBG_PHIs, and twiddled some documentation.
Wed, Jun 2
@jmorse @aprantl Sorry for the divergence, but regarding this tests reduction in terms of DI Metadata -- do you think that we can automate this process? I've written a small write-up/proposal for a utility for that purpose: https://github.com/djolertrk/llvm-metadataburn -- please let me know what you think.
Tue, Jun 1
Address feedback, delete various un-needed MIR bits and pieces, un-necessary DILocations.
Fri, May 28
LGTM plus nit.
Wed, May 26
For the record, this LGTM too. Picking a better cut-off point is probably something we can experiment with in the future.
Tue, May 25
ping here; I can see the tests here can be improved by removing attributes and a large amount of un-necessary MIR headers, otherwise IMO it's ready to go
NB: I'll probably drop this in sometimes soon, on the back of @djtodoro 's LGTM earlier
Mon, May 24
This makes sense to me: the range extension mechanism puts in DBG_VALUEs at points where the live value switches from one register to another, something that LiveDebugValues can't know later on, and often gets wrong. Instruction spans with the "wrong" scope in the middle of a block will prematurely terminate ranges -- or in the case of the prologue, it won't cover early instructions. I think there's also potential for an argument that's moved more than once in the prologue to be dropped, not completely certain.
Fri, May 21
Thu, May 20
Ping and update for @djtodoro 's comments -- some minor editing and deleting of un-necessary parts of tests.
LGTM given that I've seen a reproducer where this causes an assertion failure -- it's clearly best to avoid the assertion failure. We should re-visit this in the future as there are quite legitimate scenarios where this would be important, for example using optimised LLVM-IR with llc -O0.
LGTM. While this is a breaking change, AFAIUI the body of Dexter tests "out there" is small. If this is a serious issue for someone, we can revert and come up with a better plan.
LGTM with D102841
Wed, May 19
Minor inline nits that can be skipped, plus a question about relying on DenseMap ordering.
This works fine with our workflow at least, so a soft LGTM from me. I'd suggest waiting for a couple of days for more feedback though.
[Digging back through my inbox,]
We need a refactor here to change all places that call addBlock(, location_attr, ) to use DIEDwarfExpression
Adjust wording from review; I've added a 'tikzrender.tex' latex header and instructions on how to produce a png from a .tikz diagram. Alas, I'm more familiar with tikz than graphviz.
Mon, May 17
Hi @shchenz, is it feasible to use DIExpression::isImplicit at a higher level to filter out these kinds of expressions? I believe it's possible to parse a DIExpression before it's lowered to a DWARF expression, and there are places where we rely on the isImplicit method for correctness already. Also paging @StephenTozer who's touched this area recently.
In the code you've seen, does the debug info all or mostly consist of DBG_VALUE_LISTs now, or is it still mostly DBG_VALUE, or a mix?
I'm just trying to get a sense for how much of a degradation in debug info quality we have now.
Fri, May 14
NB: do you need this and D100270 to be pushed for you?
May 11 2021
Hmmmmm. What if we have:
May 10 2021
NB: a pre-render of the text and diagrams can be found here:
May 5 2021
This LGTM modulo one inline comment, plus: the test is in the "Generic" directory, but it is not fully generic. It depends on the armv6 triple being supported, so things like the powerpc build bots will likely trip on it. You should add an appropriate REQUIRES line.
May 4 2021
I'm not familiar with WebAssembly, but thought I'd describe how this would works in other backends -- it might inspire you to take a different route. Sorry if it doesn't apply to WebAssembly.
Apr 29 2021
Looks mechanically correct, test wanted seeing how this was missed before.
Apr 28 2021
Looks good as a starting place -- I took this for test drive, see inline comments. On a clang-3.4 build, the "PC ranges covered" statistic ticks down from 66% to 65%, however I believe that's due to rounding. Using llvm-locstats, roughly six thousand variables move out of the 100% covered bucket, or about 0.2% of all variables in clang-3.4. 0% covered goes up by a negligible amount. In my opinion: this is a reasonable sacrifice to make in the name of accuracy, and some of those dropped locations will be incorrect. Plus, there's a longer term plan for not needing this.
Apr 27 2021
Apr 26 2021
Pushed up; you might get buildbot emails about tests failing, as commits are tested in batches. I'll keep an eye on those.
Apr 23 2021
Can you help me to commit the changes as well. I don't have commit access. Thanks, @jmorse
Apr 22 2021
Looking at the output of the test, it looks like the dbg.value(undef) is in the scalar tail of the loop, rather than vector.body, right? This is slightly cross purposes to what we want, I think: the induction variable value is unavailable during the vectorized portion of the loop and should be undef, but the scalar part isn't optimised, and so the value is still available.
Apr 21 2021
LGTM with a nit
LGTM, I enjoy the minimal extension of a test to improve coverage too.
Apr 15 2021
Right you are -- another thing I didn't clock was that this test was running all of the LLVM passes (generating many instances of the coroutine function). Latest revision disables those passes so that there are only DILocalVariables for the un-coro-split function. I've confirmed this fails before the fix landed and passes afterwards.
the linked document also says "a transformation should also preserve the debug location of an instruction that is moved between basic blocks, if the destination block already contains an instruction with an identical debug location."
This is a good find; style nit and test comments inline.
Apr 14 2021
I think this is good to go, with some revisions to the test.
Apr 12 2021
NB, while I flagged that CHECK-NEXT added too much ordering, I forgot that CHECK still requires some ordering. It causes us some difficulties downstream; there's a follow-up patch here D100298 for consideration.
Apr 9 2021
LGTM with latest memory jiggery pokery.
Apr 7 2021
LGTM with a nit, and modulo @markus 's comment.
Apr 6 2021
I ended up taking this for a test drive, and found some difficulty when using the LLDB bindings -- _add_conditional_breakpoint ends up being passed a PurePath object for the file_, which is then passed to the LLDB bindings, which croak. This is easily fixed by calling str(file_) to convert the PurePath object to a string, but this might affect other debugger drivers & need some assertions adding.
LGTM, with a couple of nits, plus Orlandos comments should be addressed.
Mar 31 2021
Uh, I just figured out how mir tests work.
Mar 26 2021
Thanks for the reproducer; I think I see the same as you, when LiveDebugValues runs:
Mar 25 2021
The test now passes without the rest of the patch applied; as mentioned above, it needs to affirmatively identify the contents of retainedNodes to guarantee detection of extra entries. Alternately, use CHECK-NOT or --implicit-check-not to confirm the absence of the extra DILocalVariables.
Happily we can consult https://llvm.org/docs/HowToUpdateDebugInfo.html -- the example that you've given looks like's a good test for the "positive" case, where we should keep the source location attached to the compare instruction. The comparison doesn't get moved between blocks, so we match "When to preserve an instruction location".
I like the direction of the SelectionDAG change; reducing duplication of information is a worthy goal. My knee-jerk reaction is that we need a test for it, but I imagine it's hard to hit something that deep into DAG Combining. If you've got a reproducer to hand, it'd be good to add that as a test, if not maybe we can skip it. IMO it's also worth breaking this off from the LSR modification.
Sounds good overall, with a comment inline about the test for isSignedConstant. IMO, there should be some test coverage of expressions that shouldn't be recognised as a signed constant too.
FTR, the patch LGTM. I haven't really been following the development of debugify in detail so I have no feeling for whether this is good from a high level; if you're confident the mailing list discussion finished with this being the right direction, I'd say this is ready to land.
Mar 24 2021
There isn't a lot of difference to how debug-info is updated, as far as I can see, but the summary says a fault is resolved. Is this patch just masking the debug-info issue by doing more propagation? (I might have missed something in the implementation).
Hmmmm, do you have a reduced reproducer in llvm-ir that could go in a bug report? There are a number of things that could be going on here, and we can't be sure which without an example.
When looping over the debug users of instructions and replacing the dbg.values operands, you should now use the "replaceVariableLocationOp" helper instead -- this avoids re-creating a dbg.value intrinsic, and should handle variadic variable locations (the patches for which are 95% landed) seamlessly.
Drive-by review: some minor nits commented on, looks very reasonable overall.
Mar 18 2021
To my mind, the two attribute names, _conditional_bp_ranges and _conditional_bps are quite similar -- but AFAICT the former is a on-initialization record of where conditional bps should be placed, while the latter is a "live" record of conditional bp handles, is that right? IMO, better to have an indication of this distinction in the name, may I suggest _conditional_bp_handles?
Mar 17 2021
Passes on Windows just fine, thanks for the follow up!
Mar 16 2021
It turns out there's a plain old windows-has-different-slashes problem going on. In the Test object __init__ method, the call to os.sep.join in this patch produces the string "subdir\ccc.txt", which won't match "subdir/ccc.txt" that you load from the the .lit_test_times.txt file. That then leads to no time being loaded for ccc.txt, and it gets mis-ordered.
I'm experiencing reorder.py failing intermittently on a Windows system -- it seems to pick up the .lit_test_times.txt produced in the build directory for the Inputs tests after the first run, and use that for ordering. And seeing how that just contains noise, the Inputs tests are then run in the wrong order. Is this supposed to be accounted for already?
Mar 15 2021
Mar 11 2021
Mar 9 2021
In principle this looks good, but as @djtodoro points out it needs a test.
Mar 8 2021
Feb 24 2021
LGTM, but pls2leave it 24 hours or so in case @rnk chimes in.