[PATCH] D58453: [DebugInfo][CGP] Limit placeDbgValues movement of dbg.value intrinsics

Mon Jul 15 06:14:48 PDT 2019

jmorse added a comment.

> ! In D58453#1582991 <https://reviews.llvm.org/D58453#1582991>, @yurydelendik wrote:
>  Looks like I misunderstood meaning of *all*. I assumed that means DBG_VALUEs the refer the vreg def of the source instruction, excluding DBG_VALUEs that refer the same vreg def by the above instructions (in the same block). The test result above shows that collectDebugValues locates all DBG_VALUEs for the instruction even if it does not (re)define the value, but there are few instructions in the same block above that do. Is this the intended effect of this patch? If yes, we probably need to create more robust collectDebugValues-like method that will track lifetime of DBG_VALUEs.

Indeed, that's the intention. I guess a clearer way of saying this would have been that collectDebugValues will be much less useful after leaving SSA form, because from that point onwards one must consider register definitions, rather than just the register itself, to know what a DBG_VALUE refers to. While this is a pain, the behaviour the rest of this patch is trying to eliminate is destroying a lot of variable location information already.

I've had a quick look at the wasm-reg-stackify pass, and to me it looks like:

- It's currently performed on a per-block basis
- Domination and LiveInterval analyses are already available

Which means that the `WebAssemblyDebugValueManager` constructor could just apply an additional filter on collected DbgValues, that the LiveInterval/ValueNumber the DBG_VALUE refers to is the defining instruction.

One problem with that implementation would be that debug intrinsics outside the current basic block may still refer to a vreg that wasm-reg-stackify operates on, a situation that placeDbgValues currently masks. I don't know enough about wasm to know if this is a serious issue.

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

https://reviews.llvm.org/D58453