[PATCH] D58403: [DebugInfo][CGP] Update dbg.values when updating memory address computations

[Jeremy Morse via Phabricator via llvm-commits]

jmorse planned changes to this revision.
jmorse marked an inline comment as done.
jmorse added a comment.

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



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4938
+    // Discard debug users in other blocks.
+    if (User->getParent() != MemoryInst->getParent())
+      continue;
----------------
bjope wrote:
> 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?
> 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).


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58403/new/

https://reviews.llvm.org/D58403