User Details
- User Since
- Mar 19 2018, 2:57 AM (148 w, 4 d)
Today
Remove needless redirect of subprogram creation; add original compile commands to test case.
Yesterday
Responding to inline comment that's now sort-of-orphaned: calling back out to constructSubprogramDefinitionDIE sounds good, I've added that to DwarfUnit::getOrCreateContextDIE with a comment.
Change interception of Subprogram creation to call back out to DwarfDebug instead of looking up CUs within DwarfUnit.
Replace regression test with David's smaller test
Wed, Jan 20
Tue, Jan 19
LGTM with comments inline. While it's a simple fix, I'm not especially familiar with coroutines, so it'd be nice if someone else could chime in before landing.
Mon, Jan 11
Tue, Jan 5
This improvement looks good and preserves more source locations. Awkwardly, as @vsk says it's quite different from the other documented rules, because it relies on the context of where the instructions are going to be placed.
Mon, Jan 4
Dec 17 2020
Dec 16 2020
I wrote:
(Nabeel and I share a corporate overloard, we should leave this a little more time in case anyone else wants to chip in).
Dec 14 2020
LGTM, although couldn't you also just swap the basictest.ll DEBUGIFY-NEXT to be a DEBUGIFY-NOT: call void @llvm.dbg.value and not add the extra test.
LGTM
LGTM
Dec 10 2020
Could you upload the patch with context please, as it's easier to navigate the changes. Instructions can be found here: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
Dec 9 2020
'Tis the season to ping!
I guess that is possible. As long as both frame objects have the same Stack-ID, StackSlotColouring can merge them together. However, in that case, the sizes and offsets of those objects would both be scalable and comparing these sizes would be no different as they would be for non-scalable sizes.
Dec 8 2020
Ah, I hadn't quite lined up in my head that the offset scales as the stack grows downwards. With that in mind, I think this sounds safe -- the scalable part of the stack offset is opaque as far as LiveDebugValues is concerned, and we can just pass it around.
Dec 7 2020
LGTM, this sounds like it works just as well as before, thanks for pursuing.
Forgive my ignorance on scalable vectors; but is there definitely a scenario where the "Scalable" portion of the stack offset is nonzero, and if so could the test cover it please.
(Responding to inline comments) I think the main concern with hard-coding registers in DIExpressions at this stage is that LLVM isn't able to identify when that register is clobbered later on, and terminate the variable location. However, as you say:
I'd like to chime in and point out a slightly related scenario [0] where we have to interpret a DIExpression to work out how adding operators is going to change the meaning of the expression. I believe in the long run we need metadata describing "the attributes of this variable location" rather than packing information into DIExpressions and having to interpret it.
Dec 3 2020
A few more comments inline. I think everything in this patch works; there's still an open question in my mind of why we need to create the implicit variables so early, during instruction selection. Isn't it sufficient to leave a:
FTR, this LGTM
Nov 26 2020
LGTM too
Nov 19 2020
@StephenTozer what's the status of the latest discussions on this patch? As far as I understand it:
- DBG_VALUE may go away, but that'll be in another patch,
- There was some conclusion to whether all DBG_VALUE_LISTs should be stack_values too, can't remember what, on the mailing list,
- This patch, adding the plumbing for the DBG_VALUE_LIST, would be ready to land.
Nov 18 2020
I tried running this patch on my machine though and the tests always pass (using llvm-lit) even if I change numbers, so I don't know if something's going on with the directory test format?
Nov 17 2020
Nov 13 2020
LGTM; the point of the "Emulate VarLocBasedLDV" flag was to help prove that the overall algorithm matched what was already produced by LiveDebugValues. Seeing how the approach seems to be (broadly?) accepted / acceptable, it's to just ditch the flag altogether, it doesn't serve any purpose now IMO.
Nov 6 2020
Hi Bruno,
Thanks for the catch!
Nov 5 2020
bruno wrote:
That's a good question, I initially thought a dbg.declare dominating the BB in question would be enough, perhaps the debugging experts could help clarify what's going on.
Oct 28 2020
LGTM, anything and everything in this vein much appreciated.
Oct 27 2020
(Rebase due to me fiddling with the signature of substituteDebugValuesForInst on master).
LGTM; thanks for diving in!
Oct 23 2020
(rebase again to see if phab will accept more information about base commits and the like)
Sorry it took so long to get back to this,
(Rebase now that I've dropped some "REQUIRES: X86" lines in the modified tests...)
Oct 22 2020
Rebase to account for me gluing patch series together; one or two new code paths crop up where one has to decide whether to try and use an entry value OR register a debug use-before-def.
Rebase onto existing patches to make this applicable through arc; one line marked "future work" in livedebugvalues_instrref_tolocs.mir now has a variable location, as this is the future work that implements it.
(Update for a conflict with current master)
NB: I said in the test:
LGTM, I agree the subregister index on any undef DBG_VALUE is meaningless.
(Revise to address comments)
Oct 21 2020
Oct 20 2020
Oct 19 2020
(Adding debug-info),
Oct 16 2020
I've reverted this and the follow-up patch in rG0a7f41739fd994a50 as it continued to fail tools/llvm-cov/warnings.h on Windows-based platforms.
Oct 15 2020
Updated patch, changes signature of substituteDebugValuesForInst to take a "number of operands to look at" argument, defaults to "all of them".
After landing this, bkramer pointed out one of the assertions in substituteDebugValuesForInst was ineffective. Fixing that shows that some callers (see patch 5 in this series) do so with different instruction signatures (i.e., different types and different numbers of operands).
Hrrmmm, that smells fishy; thanks for the warning sqelching, I'll follow it up.
The variable location changes LGTM, I otherwise defer to Djordje who's made a request.
Oct 14 2020
ping; (also remove from summary the reference to additional tests needed, as these are now in the patch).
Oct 13 2020
Oct 12 2020
This completely fell off my radar sorry,
Assuming I understand the history correctly, this patch in itself won't introduce any inaccuracy into debug-info, and has a motivating performance example, so LGTM.
This fixes things for us, and by limiting to one optimisation pass it's much less brittle for the future -- much appreciated!