- User Since
- Mar 19 2018, 2:57 AM (131 w, 2 d)
LGTM; I guess it'd be better if SCEV exposed a helper method, rather than this patch having to expose one, but that's well out of scope.
Mon, Sep 21
This is looking good, a few nits inline. Some slightly broader questions: do you know if there's any risk of debug-info affecting decisions made by SCEV, i.e. causing codegen to change when compiling -g? I don't have any reason to believe that could be the case, but it's a fear in the back of my mind.
Mon, Sep 14
Fri, Sep 11
Thu, Sep 10
Revise some dodgy comments, cheers!
Address the loop matter I mention at the end of the review summary -- I've convinced myself that we only need to validate very simple loops in this context, because LLVMs tail-duplicator doesn't substantially rewrite functions, it only duplicates bits of it.
Address comments; add a test for creating DBG_PHIs for PHIs that happen in spill slots.
Fri, Aug 28
Wed, Aug 26
Thanks to all the reviewers in this series for their time!
Tue, Aug 25
Aug 22 2020
NB: landing now-ish as recent feedback is all nits, and in the "main" patch (D83047) we've come to the conclusion that this should land guarded by an experimental switch (which is what this patch does).
Aug 21 2020
Rebase to account for some whitespace differences.
Early-exit to avoid un-necessary indentation.
Remove some needless comments and some needless curly braces.
Aug 20 2020
Rebase for rG60433c63acb7 touching something near this patch, no real change.
Add a test for the driver -Xclang option. As far as I understand this, we're just checking that the switch is accepted by the driver, not that it does anything in particular (please correct me if I'm wrong).
Address @djtodoro 's comments (remove header, add C++ indicator, clang-format).
Aug 19 2020
Add another call to substituteDebugValuesForInst that I missed (line 681).
Aug 18 2020
Aug 12 2020
NB: on the topic of LowerDbgDeclare, it was discussed extensively in PR34136 (CC @rnk) , and there's actually a plan for eliminating its behaviour there  from about two years ago, which we were eventually going to look at (CC @chrisjackson).
No real additional comments on this or the 2nd patch -- for further progress though I think you need to stimulate some discussion in the llvm-dev "DW_OP_implicit_pointer design/implementation in general" thread, to convince David / Adrian that this patch series addresses their concerns from the earlier discussion.
were this series to land today, there would be some variable locations dropped
Switch a loop to being ranged-based.
Aug 11 2020
Aug 10 2020
Aug 7 2020
Here's the latest revision; in this change I've suppressed use of the "zero LocIdx": all location indexes now mean the same thing. I've done this by scattering Optional around a variety of places. There's still one rough edge, which is that the IndexedMaps in MLocTracker want a reserved value to signify a mapping that doesn't exist -- I've added an "illegal" maximally-valued LocIdx for that, which is only ever generated inside the IndexedMap. This too could be eliminated by using a std::map instead, if we're aiming for complete clean-ness at this stage.
Jul 30 2020
I see what seem to be changes to codegen here (see inline comments), is that intentional? And if so, why is that needed for DW_OP_LLVM_explicit_pointer to work?
Suggested on llvm-dev: I reckon you also need to add logic to the ConvertDebugDeclareToDebugValue helpers too, or otherwise factor them in. This patch only covers the two fast-cases that mem2reg considers, single store allocas and single block allocas, I believe (98%) everything else goes through ConvertDebugDeclareToDebugValue.
Could you elaborate on why module-level information needs to be preserved? As mentioned on llvm-dev, I think you might have missed a place where promotion from stack to SSA/values happens -- IMHO we shouldn't add module-level debuginfo-tracking unless it's absolutely necessary.
I sense this is the bulk of the work; and it looks good. I've posted on llvm-dev asking whether the creation of artificial variables could instead be deferred til DWARF emission. That'll avoid anything having to deal with a new form of DBG_VALUE, and us having to think about their lifetimes. Plus, there's greater scope for de-duplication, as I understand it this patch will generate a new artificial variable for each dbg.value, for each level of indirection.
Similar story to the parent patch, could you put a file-comment into the test explaining it's a round-trip test?
Hi @alok, would you be able to re-title the reviews in this stack to indicate what's in the patch itself? That'll make it easier to navigate.
Jul 27 2020
Misery: unfortunately diff 280460 undid the contents of the previous revision (adding isPHI helpers etc). This latest update restores that work -- please diff between this update (280920) and 280205 to see the vlocJoin rework. Sorry for the extra bother.
Jul 24 2020
Here's the correction for vlocJoin. Note that I've added tests in D83054 as they're not runnable from this patch (ooof).
Add three new tests of interesting (TM) LiveDebugValues input, which I'll refer to in D83047:
Jul 23 2020
[Bah, comments didn't get submitted]
Thanks for all the input -- this update addresses more of the feedback so far, although I've left a few things for future update(s). As mentioned above, there's some sketchyness with vlocJoin, which I now have a fix for, which I'll hopefully fold into this revision today or tomorrow.
Jul 15 2020
Should be a matter of
Jul 14 2020
(Drive-by review) Thanks for the patch -- it looks good, and generating legal machine code is definitely better than illegal,
On the topic of this implementation in general: the more I look at the vlocJoin method, the more I'm convinced it's incorrect. While it does produce an identical binary when stage2 building clang, I think it's only correctly dealing with the kind of control flow that C/C++ produces. If you expose it to weirder control flow, like these MIR files:
On the topic of entry values, I believe the value based tracking should make identifying entry values trivial -- all entry value numbers will be identifiable (as a ValueIDNum) with "BlockNo=0" and "InstNo="0", indicating a value defined before the first instruction of the first block. Ideally, the logic would be after machine-value-numbers and variable-values are propagated, when final locations are picked, and would look like this:
- This variables value is an entry value (BlockNo=0,InstNo=0),
- That value isn't available in any machine location right now
- I will emit a DBG_VALUE containing a DW_OP_LLVM_entry_value expression.
Right now I've got some extra work on this (InstrRefBasedLDV) patch to be done, more on that shortly, and a patch series adding some initial DBG_INSTR_REF support to upload. I should be able to prod the entry-value situation after that.
Jul 8 2020
Jul 7 2020
- Replace a std::pair with a DbgValueProperties class,
- Replace some Densemaps of LocIdx with std::map, for initial cleanliness,
- Rename ValueRec to DbgValue and make two exclusive fields a union,
- Use an Optional<ValueIDNum> instead of in-band signalling when it's an invalid result,
- Address additional assorted feedback.
(Rebase, only affecting LiveDebugValues.cpp)
(Rebasing, only affects LiveDebugValues.h)
Move include to start of file; was on autopilot sorry.
Thanks for the feedback,
clang-format, add file headers, address some review comments
Jul 6 2020
Jul 2 2020
NB: in D83054 I've added livedebugvalues_many_loop_heads.mir which contains a worked example (aka pathological input) that exercises the lattice-descent code for machine value numbers. It's definitely the ugliest part.
Jun 25 2020
And yet, the variable was allocated to a register, and the variable's location information pointed to the correct instruction range.
Inadequacies in our ability to represent the scope properly shouldn't cause us to eliminate *correct* location information for variables.
Jun 23 2020
FWIW: I'd enjoy this patch landing. AFAIUI there's no meaningful information communicated to the DWARF consumers by out-of-scope ranges (even if it indicates a compiler bug somewhere), and we may as well save them disk space and debugger-load-time.
Jun 18 2020
For the record, this all LGTM.
(Ah yeah, Tom is away for a bit),