[PATCH] D79428: [WebAssembly] Added Debug Fixup pass

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 9 inline comments as done.
aardappel added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp:74
+    // We may insert into this list.
+    for (auto MII = MBB.begin(); MII != MBB.end(); ++MII) {
+      MachineInstr &MI = *MII;
----------------
dschuff wrote:
> style nit:
> LLVM style would be something like
> `for (auto MII = MBB.begin, MIE = MBB.end(); MII != MIE; ++MII)`
> 
We are inserting new instructions, so depending on the container its better to be calling end() here every time?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp:97
+          ++TMII;
+          BuildMI(*MI.getParent(), TMII, MI.getDebugLoc(),
+            TII->get(WebAssembly::DBG_VALUE))
----------------
aheejin wrote:
> dschuff wrote:
> > So I guess this is building  DBG_VALUE which sets the value to register 0, which clears it? Why advance the iterator by 2? Wouldn't we need to keep advancing until the use of this def? Or until it comes off the stack?
> In a simple scenario that a register is used right after it is stackified (as in the test case in this CL), it can be iterator+2, but
> - Does this handle cases where multiple values are stackified and later used, i.e., does this handle when stack depth > 1?
> - Even in one-def / one-use scenario, is it guaranteed that the popping instruction is right after `DBG_VALUE`? There can be other instructions that do not consume stack values in between. Also there can be other debug instructions and label instructions.
> 
> I think you need to find the 'use' instruction of the given register here, using some functions like [[ https://github.com/llvm/llvm-project/blob/dee4cbcd479f075ae33a8d3841fedde388c45782/llvm/include/llvm/CodeGen/MachineRegisterInfo.h#L451-L477 | these ]] in `MachineRegisterInfo`, I guess.
> 
> And there are [[ https://github.com/llvm/llvm-project/blob/9b509bca858cbc37ab8e36383f2550d5e2f8a312/llvm/include/llvm/CodeGen/MachineInstrBuilder.h#L428-L459 | overloaded `BuildMI` functions ]] that are designed to be used to create `DBG_VALUE`.
You are both right, this should be at the use not at the def.
Will add comments to make it clearer too.
Will use DBG_VALUE variant.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp:118
+          // See also: maybeRewriteToDrop.
+          if (MO.isReg() && !MO.isDead() && MFI.isVRegStackified(MO.getReg())) {
+            Stack.push_back(MO.getReg());
----------------
aheejin wrote:
> I'm not sure we should exclude the condition `MO.isDead()` here.
> 
> There can be dead registers (and `drop`s) in normal scenarios in which we don't run `-wasm-disable-explicit-locals`. Shouldn't we preserve `DBG_VALUE`s between the value-pushing instruction and the subsequent `drop`? For example, when `@somefunction` pushes one value onto the stack,
> ```
> call @somefunction
>                                <-Here we need to preserve debug values, no?
> drop
> ```
> In general we can't even guarantee that the `drop` will follow right after the value-pushing instruction. So it is possible:
> ```
> call @somefunction
> call @somefunction
> drop
> drop
> ```
> 
> The problem of `-wasm-disable-explicit-locals` is it does not insert necessary `drop`s, and thus making the stack invalid, as you saw. How about not running this pass entirely when `-wasm-disable-explicit-locals` is given? I mean, in WebAssemblyTargetMachine.cpp, we can do something like
> ```
>   // Some comments about why -wasm-disable-explicit-locals matters
>   if (!WasmDisableExplicitLocals)
>     // Fix debug_values whose defs have been stackified.                           
>     addPass(createWebAssemblyDebugFixup());      
> ```
> To do this, we need to move `WasmDisableExplicitLocals` from WebAssemblyExplicitLocals.cpp to WebAssemblyTargetMachine.cpp, but it's no big deal.
That's what I had originally, disable the pass when Explicit Locals is off. But I thought this was more elegant since I didn't want to disable a whole bunch of tests unnecessarily. I can revert it, though.

Note that this only happens in the case of an unused variable, but yeah, I guess worth preserving.


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

https://reviews.llvm.org/D79428