[PATCH] D138943: [WebAssembly] Enable LiveDebugValues analysis

[Derek Schuff via Phabricator via llvm-commits]

dschuff 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()
----------------
does SpillLocation make sense when the loc is a register kind?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:742
-                      [](auto RegNo) {
-                        return RegNo < LocIndex::kFirstInvalidRegLocation;
-                      }) &&
----------------
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?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:2105
+      Regs.insert(MO.getReg());
+      if (MI.getMF()->getProperties().hasProperty(
+              MachineFunctionProperties::Property::NoVRegs))
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138943