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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
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);
----------------
tlively wrote:
> aheejin wrote:
> > - 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?
> I initially tried to handle this as part of the existing logic, but kept running into assertion failures in the SelectionDAGBuilder. Eventually I just cargo culted what AArch64 was doing for its tail call lowering (including the MVT::Glue) and everything worked. According to `MachineValueType.h` Glue "glues nodes together during pre-RA sched" so maybe it's for gluing the call to a subsequent return node? I'm not entirely sure. I'll try playing around with this a bit more to see if I can get rid of the glue.
I see. It sounds requiring `MVT::Glue` actually makes sense in that case. But I still wonder why can't we handle this in the existing logic? Aren't the opcode and `MVT::Glue` only different things from the normal workflow? And don't we need `InTyList` as well, as in the normal flow?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:82
+    "return_call    \t$callee", "return_call\t$callee", 0x12>,
+  Requires<[HasTailCall]>;
 
----------------
tlively wrote:
> aheejin wrote:
> > aheejin wrote:
> > > - Why is the name return call (or ret_call) and not tail call?
> > > - Are there no tail calls that return values?
> > Oh I checked the spec and it says `return_call`. Nevermind the first thing
> In wasm, `return_call` is not just a call that happens to be in a tail position, but actually a return and a call rolled into a single action. So a `return_call` can be moved by optimizations in any way that a return could be moved.
> 
> Since the instruction performs a return, it is impossible for it to produce a value that is consumed in the current context, which is why it is not modeled as returning a value. In Binaryen this is clearer because `return_call` would have type unreachable.
Hmm, I see. Then I think we'd need `isReturn = 1` for these instructions.


================
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
 
----------------
aheejin wrote:
> 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`.
ping


================
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() {
----------------
tlively wrote:
> aheejin wrote:
> > How is this `musttail` supported in fastisel, while we have no support yet? Does that fall back to the normal isel?
> Yes, musttail causes fastisel to fall back to normal isel. I tried adding fastisel support for tail call but it didn't work out. Maybe I was missing some glue...
Then how about putting a TODO comment in WebAssemblyFastISel.cpp or somewhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62877