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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp:97
+          // variable since later we need it construct another one on pop.
+          Stack.back().DebugValue = &MI;
+        }
----------------
I might be mistaken, but I'm not sure it is guaranteed that `dbg.value` instruction follows right after the corresponding def instruction. For example, this seems possible, in which `dbg.value` does not follow right after its def:
```
r0 = ...
r1 = ...
...
dbg.value r0, ... ;; at this point stack size is 2 (depth is 1), but r0's depth should be 0
dbg.value r1, ...
```

In that case it seems we can't use the stack depth as a target index.? (We may need to search the whole stack to find the matching register.)

It would be good to add a test case for this kind of case as well.

Also typo in the comment: need it construct -> need it to construct


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyDebugFixup.cpp:113
+              ++TMII;          // right after this instruction.
+              BuildMI(*Prev.DebugValue->getParent(), TMII, MI.getDebugLoc(),
+                      TII->get(WebAssembly::DBG_VALUE), false, Register(),
----------------
Nit: If we do
```
BuildMI(*Prev.DebugValue->getParent(), std::next(MII), ...
```
We don't need a separate `TMII` variable.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyTargetMachine.cpp:56
+    " instruction output for test purposes only."),
+  cl::init(false));
+
----------------
Nit: clang-format


================
Comment at: llvm/test/CodeGen/WebAssembly/stackified-debug.ll:58
+  ret void, !dbg !22
+}
+
----------------
Nit: We may not need `bitcast`s and `tail call`s. How about making this simpler by something like
```                                                                           
define void @foo() {                                                             
entry:                                                                           
  %call = call i32 @input()
  call void @llvm.dbg.value(metadata i32 %call, metadata !16, metadata !DIExpression()), !dbg !19                                                  
  %call1 = call i32 @input()
  call void @llvm.dbg.value(metadata i32 %call1, metadata !17, metadata !DIExpression()), !dbg !19                                            
  call void @output(i32 %call, i32 %call1)                                       
  ret void                                                                       
}                                                                                
                                                                                 
declare i32 @input()                                                             
declare void @output(i32, i32)
```
? (This does not include `llvm.dbg.value`; they have to be added)
Also it looks we don't need `attributes` and `local_unnamed_addr`s for this test.

Also it'd be better to add a test case when `llvm.dbg.value` is not right after its corresponding def instruction.


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

https://reviews.llvm.org/D79428