[WebAssembly] Undef invalid DBG_VALUEs after RegColoring

[aheejin]

Summary

After register coalescing, some DBG_VALUEs can have incorrect info.
For example, if a DBG_VALUE has a register operand %0, but it
resides in a live range of %1, it has incorrect info after %0 and
%1 are coalesced. See the comments for more details.

This does not have meaningful changes on our variable debug info
coverage or compilation time, which is good news.

[aheejin]

Fix tests

[aheejin]

Make LLVM_ENABLE_EXPENSIVE_CHECKS work

[dschuff]

llvm/test/DebugInfo/WebAssembly/dbg-value-reg-coloring.mir

55

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...

57

why are the NOPs here?

[aheejin]

llvm/test/DebugInfo/WebAssembly/dbg-value-reg-coloring.mir

55

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_VALUEs. 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_VALUEs 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_VALUEs (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_VALUEs 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.

57

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).

[aheejin]

Use different variables for different regs

[aheejin]

Gentle ping 😀

[dschuff]

oops sorry, LGTM, thanks for the explanation