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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Nice that this approach works!

- Please clang-format
- Can we have some more test cases other than one-def/one-use case? Like, when we have multiple stackified values at a time and uses them in one or multiple instruction, or when we have multivalue calls. I guess some tests in reg-stackify.ll or multivalue.ll can be adapted here.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp:79
+        // Also check if not a $noreg.
+        if (MO.isReg() && MO.getReg() && MFI.isVRegStackified(MO.getReg())) {
+          // Found a DBG_VALUE with a stackified register we will
----------------
Nit: `MO.getReg()` can be `MO.getReg().isValid()` to be more readable


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp:97
+          ++TMII;
+          BuildMI(*MI.getParent(), TMII, MI.getDebugLoc(),
+            TII->get(WebAssembly::DBG_VALUE))
----------------
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`.


================
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());
----------------
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.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp:988
       for (MachineOperand &MO : MI.defs()) {
-        if (!MO.isReg())
+        if (!MO.isReg() || MO.isDead())
           continue;
----------------
I don't think we need to exclude `MO.isDead()` here, regardless of what we decide to do with ExplicitLocal pass above, because this code runs before ExplicitLocal pass, so this will not fail anyway.


================
Comment at: llvm/test/CodeGen/WebAssembly/stackified-debug.ll:23
+; CHECK:   .int8	2                       # 2
+; CHECK:   .int8	0                       # 0
+; CHECK:   .int8	159                     # DW_OP_stack_value
----------------
Is this the encoded stack depth? Then how about mentioning that in the comment, like
```
; CHECK:   .int8  2                       # TI_OPERAND_STACK (2)
; CHECK:   .int8  0                       # Stack offset
```


================
Comment at: llvm/test/CodeGen/WebAssembly/stackified-debug.ll:28
+
+source_filename = "stackified.c"
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
----------------
Nit: This can be deleted


================
Comment at: llvm/test/CodeGen/WebAssembly/stackified-debug.ll:30
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown-wasm"
+
----------------
Nit: No need of `-wasm`


================
Comment at: llvm/test/CodeGen/WebAssembly/stackified-debug.ll:32
+
+define hidden void @foo() local_unnamed_addr #0 !dbg !12 {
+entry:
----------------
Nit: No need of `hidden`


================
Comment at: llvm/test/CodeGen/WebAssembly/stackified-debug.ll:50
+attributes #3 = { nounwind readnone speculatable willreturn }
+attributes #4 = { nounwind }
+
----------------
Do we need these attributes for debug value testing? If not we can remove all these I think (or leave only the necessary ones)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79428