Page MenuHomePhabricator

[DebugInfo][CGP] Update dbg.values when updating memory address computations
AcceptedPublic

Authored by jmorse on Feb 19 2019, 1:15 PM.

Details

Reviewers
aprantl
bjope
vsk
Summary

One of CodeGenPrepare's optimisations is to duplicate address calculations into basic blocks, so that as much information as possible can be folded into memory addressing operands. This is great -- but the dbg.value variable location intrinsics are not updated in the same way. This can lead to dbg.values referring to address computations in other blocks that will never be encoded into the DAG, while duplicate address computations are performed locally that could be used by the dbg.value. Some of these (such as non-constant-offset GEPs) can't be salvaged past.

Fix this by, whenever we duplicate an address computation into a block, looking for dbg.value users of the original memory address in the same block, and redirecting those to the local computation.

Diff Detail

Event Timeline

jmorse created this revision.Feb 19 2019, 1:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 1:15 PM
aprantl accepted this revision.Feb 19 2019, 1:45 PM
This revision is now accepted and ready to land.Feb 19 2019, 1:45 PM
bjope added inline comments.Feb 20 2019, 12:17 AM
lib/CodeGen/CodeGenPrepare.cpp
4946

What happens if there is an earlier dbg-use of Repl (before MemoryInst) in this BB. Is there a risk that we introduce a dbguse-before-def scenario here? Maybe we need to check if MemoryInst dominates User here? Or is such input IR unlikely in reality?

There seems to be lots of cases above (for example reusing an earlier computation for SunkAddr), so maybe the important check is that SunkAddr dominates User. But as you understand from the question marks above, I haven't figured out if that always is guaranteed here.

Notice that I do not think about scenarios when the input IR already is "broken" (dgb-use before def) here. Just some worrying that we might introduce dbg-use-before-def here.

Actually the test case that you added has the dbg.value before the load in the BB that we sink the address computation into (the scenario I'm talking about). Isn't the address computation added just before the load? Is perhaps the test case working due to placeDbgValues moving the dbg.value?

jmorse planned changes to this revision.Feb 20 2019, 9:08 AM
jmorse marked an inline comment as done.

It turns out the validity of this change relies on placeDbgValues reordering (curses), a small amount of extra juggling will be required.

lib/CodeGen/CodeGenPrepare.cpp
4946

Actually the test case that you added has the dbg.value before the load in the BB that we sink the address computation into (the scenario I'm talking about). Isn't the address computation added just before the load? Is perhaps the test case working due to placeDbgValues moving the dbg.value?

Aha, you've got it in one there, I was inadvertently relying on placeDbgValues. I'd placed the dbg.value at the top of the block and assumed optimizeMemoryInst did-the-right-thing because the duplicated memory insts appeared above the dbg.value. (Should be easily fixable).

jmorse updated this revision to Diff 188909.Mar 1 2019, 7:09 AM

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.

This works because CGP already keeps a cache of sunk addresses for each block, and we walk forwards through the block (which optimizeMemoryInst relies on anyway). Thus, we will only rewrite dbg.value operands for instructions after the sunk memory computation has been produced.

The test case will need to be updated when placeDbgValues gets limited: the first dbg.value will no longer be hoisted up a block, and the second will no longer go up one inst.

This revision is now accepted and ready to land.Mar 1 2019, 7:09 AM