[PATCH] D62877: [WebAssembly] Implement tail calls and unify tablegen call classes

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

Nice!

- Does the LLVM guarantee tail calls to be placed in the positions in which they can be really tail calls? Can this property change when we move code around in our backend, so tail calls cannot be executed as tail calls anymore?
- It seems we need to update the Tail call optimization <http://llvm.org/docs/CodeGenerator.html#tail-call-optimization> section in the Code Generator doc too.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:788
+  if (CLI.IsTailCall) {
+    SDVTList NodeTys = DAG.getVTList(MVT::Other, MVT::Glue);
+    return DAG.getNode(WebAssemblyISD::RET_CALL, DL, NodeTys, Ops);
----------------
- Why do we need `MVT::Glue`?
- Wouldn't it be better we handle this with the normal call instruction generation flow, rather than treating it as a special case?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:26
 
-multiclass CALL<WebAssemblyRegClass vt, string prefix> {
-  defm CALL_#vt : I<(outs vt:$dst), (ins function32_op:$callee, variable_ops),
-                    (outs), (ins function32_op:$callee),
-                    [(set vt:$dst, (WebAssemblycall1 (i32 imm:$callee)))],
-                    !strconcat(prefix, "call\t$dst, $callee"),
-                    !strconcat(prefix, "call\t$callee"),
-                    0x10>;
+multiclass CALL<ValueType vt, WebAssemblyRegClass rt, string prefix,
+                list<Predicate> preds = []> {
----------------
- Do `vt` and `rt` have to be separate?
- If they are the same, `prefix` looks always `vt` + `.`, so maybe we don't need `prefix` anymore?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:71
+let IsCanonical = 1 in {
+defm CALL_VOID :
+  I<(outs), (ins function32_op:$callee, variable_ops),
----------------
Probably not related to this CL, but I wonder maybe we should also merge this into a unified CALL instruction when we have calls that return multiple values.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:82
+    "return_call    \t$callee", "return_call\t$callee", 0x12>,
+  Requires<[HasTailCall]>;
 
----------------
- Why is the name return call (or ret_call) and not tail call?
- Are there no tail calls that return values?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td:102
+                              SDT_WebAssemblyCall0,
+                              [SDNPHasChain, SDNPVariadic]>;
 def WebAssemblybr_table : SDNode<"WebAssemblyISD::BR_TABLE",
----------------
Nit: stray indentations


================
Comment at: llvm/test/CodeGen/WebAssembly/tailcall.ll:2
+; RUN: llc < %s -asm-verbose=false -verify-machineinstrs -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers -mattr=+tail-call | FileCheck --check-prefixes=CHECK,SLOW %s
+; RUN: llc < %s -asm-verbose=false -verify-machineinstrs -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers -fast-isel -mattr=+tail-call | FileCheck --check-prefixes=CHECK,FAST %s
 
----------------
It looks like we don't support tail calls in fastisel yet, right?
- Do we have a plan to support tail calls in fast isel too? Then putting TODO somewhere in the fastisel code would be good.
- If `FAST` version here is the same as non-tail version, do we need to check that explicitly in FileCheck before we have the actual fastisel support? The same for `call.ll`.


================
Comment at: llvm/test/CodeGen/WebAssembly/tailcall.ll:14
+; CHECK-LABEL: recursive_musttail_nullary:
+; CHECK: return_call recursive_musttail_nullary{{$}}
+define void @recursive_musttail_nullary() {
----------------
How is this `musttail` supported in fastisel, while we have no support yet? Does that fall back to the normal isel?


================
Comment at: llvm/test/CodeGen/WebAssembly/tailcall.ll:30
+; CHECK-LABEL: recursive_musttail:
+; CHECK: return_call recursive_musttail, $0, $1{{$}}
+define i32 @recursive_musttail(i32 %x, i32 %y) {
----------------
Maybe I'm missing something, but can this `return_call` return a value, while it does not have typed versions?


================
Comment at: llvm/test/CodeGen/WebAssembly/tailcall.ll:46
+; CHECK-LABEL: indirect_musttail:
+; CHECK: return_call_indirect , $0,  $1,  $2,  $0{{$}}
+define i32 @indirect_musttail(%fn %f, i32 %x, i32 %y) {
----------------
Nit: Stray space after `return_call_indirect`, and double spaces between args. The same for other functions below too.


================
Comment at: llvm/test/CodeGen/WebAssembly/tailcall.ll:82
 ; CHECK-NEXT: .int8 9
 ; CHECK-NEXT: .ascii "tail-call"
----------------
How about adding a test for `notail` too for coverage?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62877