[PATCH] D78818: [WebAssembly] Fix debug_value's when registers are stackified.

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

There are many places that calls `MFI.stackifyVReg`. I guess we should handle them too? Not very sure if it'll be easy to compute stack offset in all those cases though.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp:22
     MachineInstr *Instr) {
   Instr->collectDebugValues(DbgValues);
 }
----------------
IIRC this `WebAssemblyDebugValueManager` class was first written when we didn't have multivalue defs, so I think this class needs to be extended to handle multivalue defs. For a multivalue instruction, in theory each of defs can have different stack status, for example,
```
r1, r2, r3 = some_call(...);
```
In this instruction, it is possible that only r1 is stackified and r2 and r3 are not. So maybe this class now needs to take not only a `MachineInstr` but also a def register or def index or something. Also this `MachineInstr::collectDebugValues` [[ https://github.com/llvm/llvm-project/blob/82ce3347273bbc1f67f7e7307007825500588b20/llvm/lib/CodeGen/MachineInstr.cpp#L2135 | does not seem to handle ]] multiple def registers either, so we might need to fix this first.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp:61
+
+void WebAssemblyDebugValueManager::replaceWithRegister(MachineOperand &MO) {
+  for (auto *DBI : DbgValues) {
----------------
Is this function used to mark an instruction as a register before it is marked as a local in ExplicitLocals? If so maybe a comment saying this register marking is tentative would be good.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:709
 
+  unsigned stackDepth() { return static_cast<unsigned>(Worklist.size()); }
+
----------------
I don't think `Worklist.size()` is the depth of the stack. `Worklist` contains not `MachineOperand`s but ranges. Each range contains multiple `MachineOperand`s. So you have to count `MachineOperand`s in each of ranges within `Worklist`.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:951
+          WebAssemblyDebugValueManager(&*Insert).replaceWithStackOffset(
+                                                    TreeWalker.stackDepth());
           ++SubsequentDef;
----------------
I think this needs to be handled per def register (within a possibly multi-def instruction). Related to the comment in `WebAssemblyDebugValueManager`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78818