[PATCH] D138943: [WebAssembly] Enable LiveDebugValues analysis

Heejin Ahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 29 15:43:59 PST 2022


aheejin added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:694
           Out << printReg(MLoc.Value.RegNo, TRI);
+          Out << "[" << MLoc.Value.SpillLocation.SpillOffset.getFixed() << " + "
+              << MLoc.Value.SpillLocation.SpillOffset.getScalable()
----------------
dschuff wrote:
> does SpillLocation make sense when the loc is a register kind?
OMG, I copy-pasted this from `SpillLocKind` by mistake. Nevermind.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:742
-                      [](auto RegNo) {
-                        return RegNo < LocIndex::kFirstInvalidRegLocation;
-                      }) &&
----------------
dschuff wrote:
> So I assume this is being removed because we are using VRegs instead of physregs, and VRegs have arbitrary large unsigned values (greater than 1 << 30). 
> Looking at what that might mean: It seems that `LocIndex` is relying on this in some way.
> Aside from there, the other place register values are checked against `kFirstInvalidRegLocation` is in `getUsedRegs`, where it seems to only collect values in the valid physreg range. so it seems that `getUsedRegs` will never return VRegs. It seems that `getUsedRegs is only used in the case of RegMask machine operand (which IIRC wasm doesn't use and which only make sense for physregs anyway).
> 
> So... I guess this seems like overall it should work? It does seem odd that we are using LocIndex but violating one of its primary assumptions. Is there anything else that could potentially go wrong?
I deleted this thinking simply like 1. this assertion failed on Wasm and 2. Wasm used virtual registers, so the assertion failure made sense. But yeah, this doesn't feel very satisfactory after all.

I don't think this will cause any functional problems in other archs. They are all using all physical registers after all. (This analysis runs at the very end of the backend pipeline.) But the funny thing is, Wasm is NOT supposed use `RegisterKind` at all. What does it even mean when debug info resides within a virtual register (which doesn't really exist in Wasm physically) at the end of compilation? All usable debug info should be either in a local or a stack location, or an immediate. So encountering a (virtual) register here in Wasm basically means it is a dangling debug info, which will be ignored when written to binary.

I'm not sure we can consider this kind of dangling `DBG_VALUE`  hanging around a critical bug to fix. While it is a bug, it can happen quite often, when a pass does not handle its corresponding `DBG_VALUE` when transforming/deleting/splitting a normal instruction. Preserving as much as `DBG_VALUE` is more like an optimization problem; not preserving some of them doesn't mean the program should crash.

So in short, I had to disable this to make it run on many Wasm programs, which have some dangling `DBG_VALUE`s at the end. I think maybe more tidy solution here would be to make the pass ignore register-based `DBG_VALUE` in case of Wasm, and re-enable this assertion..? But to do that we have to check the architecture at some point, which feels a little ugly to me, but maybe that's better than this.

WDYT?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:2105
+      Regs.insert(MO.getReg());
+      if (MI.getMF()->getProperties().hasProperty(
+              MachineFunctionProperties::Property::NoVRegs))
----------------
dschuff wrote:
> would it also be correct to condition this on MO.getReg() being a physical rather than a virtual reg? If so, I think something like that would be clearer than the more general condition that there can't be any vregs at all.
Makes sense. Changed to check the register itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138943



More information about the llvm-commits mailing list