[PATCH] D79428: [WebAssembly] Added Debug Fixup pass
Heejin Ahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 13 05:54:34 PDT 2020
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
More information about the llvm-commits
mailing list