Currently, enabling debug information when compiling for WebAssembly crashes the backend. This commit fixes these by skipping debug values in backend passes.
Details
Diff Detail
Event Timeline
lib/CodeGen/LiveIntervalAnalysis.cpp | ||
---|---|---|
1316–1326 | This is a strange change, MRI.use_nodbg_operands() should not return operands of DebugValues (hence the _nodbg_ in the name). Why do you still need to skip them? | |
lib/CodeGen/LiveRangeCalc.cpp | ||
187–188 | This is a strange change, MRI.use_nodbg_operands() should not return operands of DebugValues (hence the _nodbg_ in the name). Why do you still need to skip them? |
You're right, it looks like the isDebug parameter wasn't being set correctly on MachineOperand's, so the nodbg iterator didn't do anything. The updated patch sets that parameter now, but I'm not sure if it's in the correct place, or if the other parameters (e.g. isUse, isKill, etc) also need to be set.
With no regalloc bits left I leave the rest of the review to webassembly people.
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp | ||
---|---|---|
261–262 | This situation should still be impossible. | |
lib/Target/WebAssembly/WebAssemblyStoreResults.cpp | ||
102–107 | wouldn't it be easier to use a use_nodbg iterator instead of doing extra checks here? |
DebugValue only has inputs so MachineOperand::IsDebug==1 should imply IsUse. Debug values should be excluded from any liveness calculation, so IsKill/IsDead/IsUndef should have no effect.
The examples are pretty wordy, could you trim them so they have less code while preserving the same checks?
The examples are pretty wordy, could you trim them so they have less code while preserving the same checks?
lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp | ||
---|---|---|
92 ↗ | (On Diff #62250) | This is the part that was missing for use_nodbg_iterands to skip? |
lib/Target/WebAssembly/WebAssemblyReplacePhysRegs.cpp | ||
---|---|---|
92 ↗ | (On Diff #62250) | Yeah, but I'm not sure if this is the right way to do it. |
I'm concerned about the changes in DbgValueHistoryCalculator.cpp. WebAssembly is currently using virtual registers essentially in the way other architectures use physical registers at this point, so it's not obvious that, eg., just skipping clobberRegisterUses for virtual registers is the right approach.
I think this suggested change would make DVHC "do the right thing" for vregs. Obviously that's just part of the way and we haven't solved any of the potential MC issues, etc. but it would serve the same purpose of not crashing, and be probably correct under the current usage.
Dan, what do you think?
lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp | ||
---|---|---|
168 | I think it's probably safe to assume that all vregs are changing in the function, because vregs are essentially function-scoped. So that probably means we don't use ChangingRegs for vregs, and this bit is OK. | |
195–196 | I think we do want to make this actually do the right thing (i.e. call clobberRegisterUses appropriately rather than not at all) for virtual registers. The code in lines 197-200 (on the left side) accounts for he fact that clobbering a preg P will also clobber any other preg that aliases P, and the code on 201-212 handles a special kind of operand called a register mask that clobbers multiple pregs. For vregs, there's no aliasing, and only one clobber is needed. So maybe here we could just add another conditional inside the one on line 195, that handles the vreg case (i.e. calls clobberRegisterUses), and put the existing code inside its else clause. | |
244 | I guess then this case would be that clobberRegisterUses would be called if the reg is virtual or in the ChangingRegs set. |
Modify DbgValueHistoryCalculator to clobber virtual registers without checking aliasing
The changes to WebAssembly-specific code here look fine to me.
Concerning the changes to lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp, I don't have a clear picture of how virtual registers will be handled in debug info yet, so I'm not confident in reviewing changes to target-independent code myself.
That said, for present purposes it may be sufficient to just make codegen not crash. That may be sufficient to motivate the patch, however if that's the case, I'd like to get it reviewed from someone who isn't also a WebAssembly developer, since this is changing code shared by all backends.
Since this patch touches both wasm and other portions of LLVM, I will split off the changes to DbgValueHistoryCalculator into a separate patch.
I tried this locally by itself, and it looks like it broke test/Codegen/WebAssembly/offset.ll, userstack.ll, cfg-stackify.ll, mem-intrinsics.ll, and byval.ll (I'm guessing it just perturbs the codegen but it's not obvious why). Also the new tests crash; presumably we should mark this patch as dependent on D22590; I'll try to push that forward.
Yeah, it's affecting the wasm codegen, which shouldn't be happening. Let me take a look.
Sorry for being late to the party... Is my assumption correct that WebAssembly used only virtual registers (because it's an SSA-based representation with an infinite register file)?
@aprantl
Yes, that's correct. (well, it's not an SSA-based representation but it does have the infinite register file). Backends are expected by various target-independent lowering code to have a few particular physical registers (primarily SP/FP), but after lowering to MachineInstr we actually replace those with virtual registers too and use vregs through all the common backend passes. Right before we emit, we actually do a re-numbering and coloring step (to try to reduce the total number of registers and map them into a 0-indexed number space).
Codegen-wise it's gone pretty smoothly; I did some refactoring on PrologEpilogInserter a couple months ago (which reminds me, I still owe a cleanup on that) but most of the passes do just fine. We will need some more work though for dealing with debug info and MC.
test/DebugInfo/WebAssembly/live-intervals.ll | ||
---|---|---|
2 | Does this work with fast-isel too? | |
2 | I think the name of the test should refer to debug values somehow since that's why it exists. | |
test/DebugInfo/WebAssembly/wasm.ll | ||
1 ↗ | (On Diff #67870) | Likewise with fast-isel and the name of the test could be more descriptive. |
No, it doesn't work with fast-isel; I believe there is a special fastLowerIntrinsicCall() hook that needs to implemented and a case for Intrinsic::dbg_declare. Since the two tests have overlapping functionality, I kept the more complex one.
Does need to work with fast-isel as well if that's on at O0 by default please.
Thanks!
-eric
To clarify, this patch fixes crashes in the backend for WebAssembly with debug information enabled. But this doesn't mean that the debug information is correct or usable, since we haven't decided on a debug format yet.
LGTM. The fast-isel check at least ensures that we don't regress from where we are now with this patch (don't crash, even if the debug info isn't right).
Seems reasonable to me. Since Matthias had some concerns originally you should let him ack it as well.
-eric
This is the stuff that was committed in rL278371, right? Can you rebase?