- User Since
- Mar 19 2018, 2:57 AM (52 w, 3 d)
Tue, Mar 19
Fri, Mar 15
Thu, Mar 14
Update to use unknown-locations for everything but promoted store instructions,
Wed, Mar 13
Many thanks for the reviews!
Tue, Mar 12
Ping -- AFAIUI we're happy with the meaning in this patch, wording and presentation still needs review?
Mon, Mar 11
Fri, Mar 8
Thu, Mar 7
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:
Wed, Mar 6
Mon, Mar 4
Re-think opening of paragraph on the risks involved when moving/altering code.
Incorporate further wording and structure feedback
Fri, Mar 1
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.
Wed, Feb 27
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.
Tue, Feb 26
Mon, Feb 25
Fri, Feb 22
Update comments in placeDbgValues to reflect its new purpose; remove code calculating the previous non-debug-instr, as we now use a dominator tree check instead.
Thu, Feb 21
Use isCopyInstr to detect copy instructions, which will catch more opportunities that just isCopy().
This problem had PR40427 hiding at the bottom of it, so this WIP isn't required.
Wed, Feb 20
It turns out the validity of this change relies on placeDbgValues reordering (curses), a small amount of extra juggling will be required.
Feb 19 2019
Feb 15 2019
Explicitly test for whether we're pre or post regalloc when deciding whether a copy can be propagated, refactor test logic.
Feb 14 2019
Sort dbg-values to be sunk to ensure determinism, sprinkle -NEXT on some CHECKS to strengthen test.
Feb 13 2019
The filtering from r353735 happily makes the sparc test change un-necessary.
Rebase onto Bjorns changes, remove now un-necessary test change.
Also, thanks for reviews!
Feb 12 2019
(Plus I added a new test to cover this behaviour, and removed some plumbing for salvageDebugUsersForDbgValues that was folded into an earlier commit).
The issue with generating new DW_OP_derefs has been resolved by r353824, as a result we no longer change existing tests with this patch.
Ping, @CarlosAlbertoEnciso how's the utility function?
Folded last comments into commit, but phab doesn't show those comments inline because of path differences it seems:
- Added comment warning against salvaging loads in salvageDebugInfoImpl
- Removed TBBA metadata and un-necessary DILocations
- New test case positively checks for an undef dbg.value corresponding to the optimised-out load.
Feb 11 2019
Feb 8 2019
This turned out to have PR40427 burried at the bottom of it, and was fixed in r352350.