- User Since
- Mar 19 2018, 2:57 AM (61 w, 1 d)
Apply review comments (change a vector size, shuffle the order of some blocks).
Fri, May 17
Nikola mentioned in D61890 that the changes to fission-ranges.ll result in reduced variable location ranges, which is unfortunate. IMO, eyeballing the test, I think the input IR we have at the moment is wrong, and it was only the bug that this patch fixes that caused us to get the output right.
The reason I offered this patch as a alternative to D61600 is that it degrades some ranges when it could leave them wider (test/DebugInfo/X86/fission-ranges.ll).
LGTM (I think I can approve?). The source IR needing no-frame-pointer-elim seems like enough reason to keep that attribute around.
Thu, May 16
Update diff to more production-ised version, will edit summary and add reviewers shortly
Wed, May 15
Just to note: the patch this is based on (r359426) was reverted in r360301 due to compile time issues. I've got a patch incoming (tomorrow) that relies on this patch to fix it: they'll need to be un-reverted / committed in a certain order and possibly all together. (We can work through that when this is approved though).
Hmmm, I get what the patch is doing, but I have the feeling that it might be the wrong place (or impossible) to fix in LiveDebugValues. Given the sample source in dbg-live-debug-values-end-range.mir, would I be right in thinking that this patch is to fix the ChangingRegs behaviour that David brought up in ?
Fri, May 10
Wed, May 8
For some reason I hadn't clocked that the IRBuilder deals with blocks in a state of partial construction/correctness (duh), and had assumed there would always be a terminator in each block, whoops.
Add comment explaining the choice of zero line numbers in getDebugValueLoc. After discussion in D61198, I've removed the clause preserving the line numbers of store instructions -- It's probably better to make it clear that the line number information in debug intrinsics is completely meaningless, and thus not try to preserve it at all. The selection of a zero line number is thus just a defence against leaky optimisations.
Remove an accidental isIndirect=true flag, my bad.
Scatter comments / address review
Tue, May 7
Tue, Apr 30
Mon, Apr 29
Fri, Apr 26
Add the test change I forgot to stage (whoops) -- the AMDGPU test here doesn't have any non-meta instructions with line numbers. Prior to this patch the debug intrinsic line numbers leak in, which the test depends on. Fix this by giving the real instructions line numbers. (The test is for placement of dbg.values rather than line numbers anyway).
Ping; I've uploaded D61198 which likely eliminates the lions share of DebugLocs-leaking-from-DbgIntrinsics. This patch would still be required however to reduce the damage from other scenarios where DebugLocs leak from debug intrinsics.
Hi, I've uploaded D61181 with a patch that might fix those regressions -- if you happen to have any spare cycles for evaluating it that'd be appreciated.
Rename variable, improve comment, sprinkle a little clang-format.
Thu, Apr 25
I've rebased this patch to be based on r358073, Davids improvements to DbgEntityHistoryCalculator. Rather than tracking non-register DBG_VALUEs separately, this patch now iterates over all open DBG_VALUE ranges and either terminates the constant ones, or clobbers the register ones. This should be the same behaviour as the previous patch.
Wed, Apr 24
NB: I've written up a sort of broad-ish summary of what's wrong with the SelectionDAG scheduling of DBG_VALUEs in PR41583 , which IMO is the cause of the unfortunate variable-location droppage described above. I've got an additional patch that should go up tomorrow.
Apr 15 2019
Thanks for the all the reviews; still outstanding is the dbg.declares-get-stashed-in-MF-objects matter, which I'll generate a follow-up for.
Apr 12 2019
Apr 9 2019
Many thanks for the reviews, I've juggled the text in the first third in response to comments,
Mar 27 2019
Maybe ISel should have put the COPY to %0 after the last DBG_VALUE (now we got a dbg-use of %15 after the last non-dbg-use of %15). Or maybe it should have used %0 instead of %15 in that DBG_VALUE. Or maybe there should be one DBG_VALUE before the COPY using %15 and one after using %0.
Mar 26 2019
LGTM, many thanks for the find and fix!
Mar 25 2019
Ping on this -- I think we all agree that this is a part of a large issue (DBG_VALUEs of non-live variables), but this patch is a step in the right direction.
Mar 22 2019
Mar 21 2019
Mar 19 2019
Mar 15 2019
Mar 14 2019
Update to use unknown-locations for everything but promoted store instructions,
Mar 13 2019
Many thanks for the reviews!
Mar 12 2019
Ping -- AFAIUI we're happy with the meaning in this patch, wording and presentation still needs review?
Mar 11 2019
Mar 8 2019
Mar 7 2019
I produced another patch (D59027, work-in-progress) that only creates undef DBG_VALUEs when the order of assignments would change... however I then realised I'd misunderstood Bjorns question here:
Mar 6 2019
Mar 4 2019
Re-think opening of paragraph on the risks involved when moving/altering code.
Incorporate further wording and structure feedback
Mar 1 2019
Incorporate recommendations on using auto from Andrea, delivered offline,
LGTM, although I'll wait a bit for more opinions
Incorporate feedback: use skipDebugInstructionsForward for better readability, iterate forwards when sinking DBG_VALUEs.
Rename new test case's file name, move it to the 'DebugInfo' directory too as that seems more appropriate.
Avoid relying on placeDbgValues for this change. We already walk (forwards) through all instructions in a block looking to optimise them, add a dbg.value visitor that rewrites the dbg.value operand if it refers to a sunk address computation.
Sprinkle some possessive apostrophes, speling, incorrect variable names
Apply review feedback, re-write the dbg.value undef example to better demonstrate the problem at hand.
Feb 27 2019
I've generated a short summary in https://reviews.llvm.org/D58726 which illustrates how I believe dbg.values are to be interpreted -- I'll chuck this at llvm-dev tomorrow if it's not obviously broken. As mentioned in the summary, it's likely that more could be said, but getting agreement on the location of a dbg.value being the location that an assignment ``happens'' would be good.
Feb 26 2019
Feb 25 2019