[PATCH] D138943: [WebAssembly] Enable LiveDebugValues analysis

[Derek Schuff via Phabricator via llvm-commits]

dschuff accepted this revision.
dschuff added a comment.
This revision is now accepted and ready to land.

This looks good to me overall with the plan for followup. Maybe we can wait to land for a bit to see if @jmorse has any followup comments.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp:75-79
-  MachineFunctionProperties getRequiredProperties() const override {
-    return MachineFunctionProperties().set(
-        MachineFunctionProperties::Property::NoVRegs);
-  }
-
----------------
aheejin wrote:
> dschuff wrote:
> > 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?
> I don't think setting `NoVRegs`will affect the functionality or make the compiler crash or anything, but in the MIR sense, we still have virtual registers after ExplicitLocals, no? We just insert `local.get/set`s so that they can be translated to stack machine Wasm instructions later.
> 
> Before ExplicitLocals:
> ```
> %0 = i32.const 3
> ...
> use %0
> ```
> 
> After ExplicitLocals:
> ```
> %0 = i32.const 3
> local.set 5
> ...
> %1 = local.get 5
> use %1
> ```
> 
> So the MIR after ExplicitLocals still contains `%0` and `%1`, which are virtual registers, even though they will not be materialized in the final Wasm code. So I'm not sure if we should set `NoVRegs` after that.
Ah, right I had forgotten that the register operands aren't removed until MCInstLower. I guess any code that runs after explicit locals just has to be careful not to mess up the relationship between the local gets/sets and their associated operands.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138943