[PATCH] D138943: [WebAssembly] Enable LiveDebugValues analysis

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp:75-79
-  MachineFunctionProperties getRequiredProperties() const override {
-    return MachineFunctionProperties().set(
-        MachineFunctionProperties::Property::NoVRegs);
-  }
-
----------------
aheejin wrote:
> jmorse wrote:
> > Doing this makes sense for WASM, but check that there are no virtual registers remains an important sanity check for other targets -- mixing vregs and physregs would definitely generate broken results. Would you be able to add an assertion somewhere else for everything but WASM? (I don't think this method can know what the target arch is).
> Added an assertion in the beginning of `LiveDebugValues::runOnMachineFunction`.
I wonder if the ExplicitLocals pass should actually set the NoVRegs property? It should hold true after that pass runs, and I don't think we add any ourselves after that?


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:2105
+      Regs.insert(MO.getReg());
+      if (MI.getMF()->getProperties().hasProperty(
+              MachineFunctionProperties::Property::NoVRegs))
----------------
aheejin wrote:
> 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.
This change can be reverted now that we've removed the VRegs, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138943