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

[Thomas Lively via Phabricator via llvm-commits]

tlively marked an inline comment as done.
tlively 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);
----------------
aheejin wrote:
> 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?
I integrated the IsTailCall path into the existing logic flow. I think this makes the code more complicated, but perhaps it is less surprising now.


================
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:
> 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
I suppose we should probably support tail calls in FastISel. I started doing that but ran into issues so I gave up. Probably worth another shot in a separate PR. I've added a TODO.

FastISel is not the same as DAG ISel because it lowers optional tail calls to normal calls while DAG ISel lowers them to tail calls. Since there is a difference I thought it worth a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62877