[PATCH] D138943: [WebAssembly] Enable LiveDebugValues analysis

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp:75-79
-  MachineFunctionProperties getRequiredProperties() const override {
-    return MachineFunctionProperties().set(
-        MachineFunctionProperties::Property::NoVRegs);
-  }
-
----------------
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.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:2105
+      Regs.insert(MO.getReg());
+      if (MI.getMF()->getProperties().hasProperty(
+              MachineFunctionProperties::Property::NoVRegs))
----------------
dschuff wrote:
> 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?
Not really, at least not without touching other parts. This function is [[ https://github.com/llvm/llvm-project/blob/83396d85495914e17746d0ea6daa56eb35de3b39/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp#L2162-L2163 | called on all instructions ]], not only `DBG_VALUE`s, so our virtual registers enter here. They will not play any role in the rest of the pass though. Apparently `DefinedRegs` which are collected in this function is only used in [[ https://github.com/llvm/llvm-project/blob/83396d85495914e17746d0ea6daa56eb35de3b39/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp#L2165 | recordEntryValue ]] function, and we don't use [[ https://dwarfstd.org/ShowIssue.php?issue=100909.1 | entry values ]]. But anyway without this `if` we will crash.

On the other hand, [[ https://github.com/llvm/llvm-project/blob/83396d85495914e17746d0ea6daa56eb35de3b39/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp#L741-L745 | the other assertion ]] I had removed and revived, is in [[ https://github.com/llvm/llvm-project/blob/83396d85495914e17746d0ea6daa56eb35de3b39/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp#L728-L762 | VarLoc::insert ]], and only runs on registers in `DBG_VALUE`s, which we removed, so we can have that assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138943