[PATCH] D65246: [WebAssembly] Do not emit tail calls with return type mismatch

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 5 inline comments as done.
tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:649
+    bool MustTail = CLI.CS && CLI.CS.isMustTailCall();
+    if (Subtarget->hasTailCall() && !CLI.IsVarArg) {
+      // Do not tail call unless caller and callee return types match
----------------
aheejin wrote:
> Would we need a test case for vararg too?
There already is one that was updated for this change.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:677
+    }
   }
 
----------------
aheejin wrote:
> Should we document about these restrictions (such as no vararg) [[ https://llvm.org/docs/CodeGenerator.html#tail-call-optimization | here ]] too, like other platforms?
Yes, I'll add documentation.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:238
+            valTypesFromMVTs(CallerRetTys, Returns);
+          }
+
----------------
aheejin wrote:
> Why should we do this? Is the return type of the tail-called function different from that of the caller? If so, wouldn't we have already disabled tail calling in ISelLowering as you did above?
Up above the return types for the current machine instruction (`Returns`) are determined from the instruction's defs. But return_calls do not have defs, so we need this extra code to record their return types by examining the caller's return type.


================
Comment at: llvm/test/CodeGen/WebAssembly/tailcall.ll:215
+; Check that the signatures generated for external indirectly
+; return-called functions include the proper return types
+
----------------
aheejin wrote:
> I'm not sure what this test is trying to test..?
This tests that we get the `ReturnType: I32` correctly. Without the change you asked about above we incorrectly determine that the return type is `none`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65246