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

[Thomas Lively via Phabricator via llvm-commits]

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:
> - 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.


================
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 = []> {
----------------
aheejin wrote:
> - 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?
yes, for simd. `vt` is `v16i8`, `v4i32`, etc. but `rt` is always V128.

`prefix` is actually `rt` in lowercase, but there is no tablegen for lowercasing a string :(. We could actually clean a lot of SIMD tablegen code up if we could properly manipulate strings, but I'm not sure that would be a welcome tablegen feature.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:71
+let IsCanonical = 1 in {
+defm CALL_VOID :
+  I<(outs), (ins function32_op:$callee, variable_ops),
----------------
aheejin wrote:
> 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.
Yes, something like that is going to need to happen. It will probably be rather tricky, given the rigidity of the SelectionDAG type system.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrCall.td:82
+    "return_call    \t$callee", "return_call\t$callee", 0x12>,
+  Requires<[HasTailCall]>;
 
----------------
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.


================
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() {
----------------
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...


================
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) {
----------------
aheejin wrote:
> Maybe I'm missing something, but can this `return_call` return a value, while it does not have typed versions?
It returns a value to the caller of the current function but does not return a value to the current function itself.


================
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) {
----------------
aheejin wrote:
> Nit: Stray space after `return_call_indirect`, and double spaces between args. The same for other functions below too.
The space is not stray, unfortunately. There is a FIXME comment in the instruction printer to remove the leading comma.

I have fixed the double spaces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62877