This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Undef invalid DBG_VALUEs after RegColoring
ClosedPublic

Authored by aheejin on May 22 2023, 10:48 AM.

Details

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.

Diff Detail

Event Timeline

aheejin created this revision.May 22 2023, 10:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 22 2023, 10:48 AM
Herald added subscribers: pmatos, asb, wingo and 6 others. · View Herald Transcript
aheejin requested review of this revision.May 22 2023, 10:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 22 2023, 10:48 AM
aheejin edited the summary of this revision. (Show Details)May 22 2023, 10:53 AM
aheejin updated this revision to Diff 524395.May 22 2023, 11:00 AM

Fix tests

aheejin updated this revision to Diff 524422.May 22 2023, 12:06 PM

Make LLVM_ENABLE_EXPENSIVE_CHECKS work

dschuff added inline comments.May 22 2023, 4:29 PM
llvm/test/DebugInfo/WebAssembly/dbg-value-reg-coloring.mir
56

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

58

why are the NOPs here?

aheejin added inline comments.May 22 2023, 5:31 PM
llvm/test/DebugInfo/WebAssembly/dbg-value-reg-coloring.mir
56

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.

58

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 updated this revision to Diff 524542.May 22 2023, 5:31 PM

Use different variables for different regs

Gentle ping 😀

dschuff accepted this revision.May 30 2023, 4:32 PM

oops sorry, LGTM, thanks for the explanation

This revision is now accepted and ready to land.May 30 2023, 4:32 PM
This revision was automatically updated to reflect the committed changes.