[PATCH] D56788: [DebugInfo][InstCombine] Prefer salvaging dbg.values over sinking them

[Jeremy Morse via Phabricator via llvm-commits]

jmorse marked an inline comment as done.
jmorse added inline comments.


================
Comment at: test/Transforms/InstCombine/debuginfo_add.ll:52
   ; CHECK-LABEL: for.body.lr.ph:
   ; CHECK-NEXT: %0 = load
+  ; CHECK-NEXT: br label %for.body
----------------
bjope wrote:
> Shouldn't there still be som dbg.value for variable !25 after the load, indicating that the variable is in %0?
> 
> Have you checked what happens in the final output? It all might depend on which live range that is longest (%start or %0), but if this load is the last user of %start we only know how to get the value of the variable !25 from %0 after the load.
> 
> I'm also not sure how well LLVM handles these dbg.value intrinsics with DW_OP_deref. What defines the end of the debug range for those? The end of the BB? The next instruction that potentially can write to memory? When the pointer register is dead? The use of DW_OP_deref (in opt) has been quite rare so far (afaik). A direct mapping to the SSA value is a more well established concept (the dbg.value is valid for the range of the SSA value, or up until the next dbg.value that indicates that the variable is somewhere else). Perhaps also limited to the current BB (or is that only for DBG_VALUE and not dbg.value? or only after de-SSA?). But when we say that the variable is in memory (and not a unique stack slot for the variable), how do we know when this isn't valid any longer when calculating debug value ranges?
> Shouldn't there still be some dbg.value for variable !25 after the load, indicating that the variable is in %0?
>
> Have you checked what happens in the final output? It all might depend on which live range that is longest (%start or %0), but if this load is the last user of %start we only know how to get the value of the variable !25 from %0 after the load.

Definitely true -- experimentally testing this on a build of clang-3.4, with the patch as it is we give 75.07% of all variables locations, and cover 45.15% of scope bytes; placing a dbg.value at the sunk location too produces 75.16% location coverage and 45.71%. Which is a small but non-trivial improvement.

The downside is that we will leave a dbg.value in each block an instruction sinks through -- and if multiple memory computations are salvaged through (load then gep then ptrcast) each one will leave a dbg.value in blocks sunk through. That being said, a few timed builds of clang-3.4 show the performance penalty as being less than 0.5%, which is well within error margins.

> I'm also not sure how well LLVM handles these dbg.value intrinsics with DW_OP_deref. [Various possibilities].

A great question, that I don't know the answer to right now. I'll try and break some examples using this patch.


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

https://reviews.llvm.org/D56788