[PATCH] D151125: [WebAssembly] Undef invalid DBG_VALUEs after RegColoring

Mon May 22 17:32:00 PDT 2023

aheejin added inline comments.

================
Comment at: llvm/test/DebugInfo/WebAssembly/dbg-value-reg-coloring.mir:55
+    ; This should be set to undef, because this is within %0's live range
+    DBG_VALUE %1, $noreg, !5, !DIExpression(), debug-location !10
+    CALL @use, %0, implicit-def $arguments
----------------
dschuff wrote:
> here's is a question to make sure I understand what's going on:
> Because the live ranges of %0 and %1 don't overlap, they get coalesced (let's call the resulting wasm local local0). So if we left the DBG_VALUE in, the DWARF would say that local0 contains the value of %1 here even though it still contains %0. Hence we just undef it out.
> Would it also be correct to move the DBG_VALUE down to below (or above?) the CONST instruction that materializes it? would that help in practice or is there usually just another DBG_VALUE for the same value?
> I guess if you said this CL doesn't meaningfully move the coverage metric, then the answer is that it it doesn't matter much in practice...
> here's is a question to make sure I understand what's going on:
> Because the live ranges of %0 and %1 don't overlap, they get coalesced (let's call the resulting wasm local local0). So if we left the DBG_VALUE in, the DWARF would say that local0 contains the value of %1 here even though it still contains %0. Hence we just undef it out.

I'm not sure if I understand what you're asking. I think I made the test confusing by using the same variable for both `DBG_VALUE`s. I changed it to use different variables now. Anyway, to rewrite the code here,
```
%0 = CONST_I32 0
DBG_VALUE %0, $noreg, !"var_a", ...
DBG_VALUE %1, $noreg, !"var_b", ...
use %0
```
`DBG_VALUE %1, $noreg, !"var_b", ...` means `var_b` at this point contains whatever `%1` contains. That can be anything. We can have `%1 = CONST_i32 999` somewhere above. This happens because `DBG_VALUE`s are not counted as 'uses' in live range analysis, so this `DBG_VALUE %1` is not considered to be extending the life range of `%1`.

And after two regs are coalesced into `%0`:
```
%0 = CONST_I32 0
DBG_VALUE %0, $noreg, !"var_a", ...
DBG_VALUE %0, $noreg, !"var_b", ...
use %0
```
Now `var_b` contains 0, where it shouldn't. The situation doesn't change even if the two regs are coalesced into `%1`:
```
%1 = CONST_I32 0
DBG_VALUE %1, $noreg, !"var_a", ...
DBG_VALUE %1, $noreg, !"var_b", ...
use %1
```
`var_b` (incorrectly) contains 0 here too. So what I'm trying to do here is, if a `DBG_VALUE` has a register, let's say, `%a`, as its operand, and if `%a` and `%b` are coalesced, and if that `DBG_VALUE` is within `%b`'s live range, it should be undefed. (It can't be both within `%a` and `%b`'s live ranges, which means two registers' live ranges overlap, in which case they wouldn't have been selected as a coalescing candidates.)

> Would it also be correct to move the DBG_VALUE down to below (or above?)

LLVM's `DBG_VALUE` update guideline recommends against moving `DBG_VALUE`s (https://llvm.org/docs/SourceLevelDebugging.html#instruction-scheduling) in order not to reorder assignment orders. One exception is when we sink a value, we can sink its `DBG_VALUE`s with it, but we should leave the original `DBG_VALUE` in place with its register set to undef. We do that here in `WebAssemblyDebugValueManager`, which is used in `RegStackify`: https://github.com/llvm/llvm-project/blob/c1fe1474d27f6fe7b8e5bfedcc9066e9a90ad85e/llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp#L295-L335
But I don't think what we do in `RegColoring` qualifies for sinking or hoisting anyway.

> the CONST instruction that materializes it? would that help in practice or is there usually just another DBG_VALUE for the same value?
> I guess if you said this CL doesn't meaningfully move the coverage metric, then the answer is that it it doesn't matter much in practice...

We can consider rematerializing certain trivially rematerializable instructions as you suggested, like `CONST_`s, but I doubt it will matter much in practice, given that this doesn't really negatively affect Emscripten benchmark (-O1)'s coverage much.

================
Comment at: llvm/test/DebugInfo/WebAssembly/dbg-value-reg-coloring.mir:57
+    CALL @use, %0, implicit-def $arguments
+    NOP implicit-def $arguments
+    %1:i32 = CONST_I32 1, implicit-def $arguments
----------------
dschuff wrote:
> why are the NOPs here?
Just to make it easier to read, in the way that above the `NOP` is %0's live range, below it is %1's live range. Deleting it doesn't change the test results (other than the `NOP` itself).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151125