[PATCH] D138943: [WebAssembly] Use LiveDebugValues analysis

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:2105
+      Regs.insert(MO.getReg());
+      if (MI.getMF()->getProperties().hasProperty(
+              MachineFunctionProperties::Property::NoVRegs))
----------------
jmorse wrote:
> aheejin wrote:
> > 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.
> This change won't hinder other targets, however you mention that virtual register don't participate in LiveDebugValues, so they don't need to be in the defined register collection AFAIUI. It might be cleaner to add the `isPhysicalRegister` condition to the `if` so that virtual registers are completely ignored.
Hoisted the `isPhysicalRegister` condition so that it's now this
```
for (const MachineOperand &MO : MI.operands())
  if (MO.isReg() && MO.isDef() && MO.getReg() &&
      Register::isPhysicalRegister(MO.getReg())) {
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138943