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

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 3 inline comments as done.
aardappel added a comment.

@aheejin thanks for the comments! As you say, given that stackifying happens across multiple passes, it may be that the approach here is going to be too clumsy/fragile. I am going to first see if a solution that works at or just before MC lowering can work instead, that way all the stackifying changes can remain as they are.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp:22
     MachineInstr *Instr) {
   Instr->collectDebugValues(DbgValues);
 }
----------------
aheejin wrote:
> 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.
> 
Ah yes, hadn't thought of multivalue :( Though maybe its worth adding multi-value support in a separate CL.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp:61
+
+void WebAssemblyDebugValueManager::replaceWithRegister(MachineOperand &MO) {
+  for (auto *DBI : DbgValues) {
----------------
aheejin wrote:
> 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.
No, it is used to change a register that was changed to a stack value back to a register. Hopefully its not necessary.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:709
 
+  unsigned stackDepth() { return static_cast<unsigned>(Worklist.size()); }
+
----------------
aheejin wrote:
> 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`.
I did think about this and somehow thought it was equivalent. Will check if I revisit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78818