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

[Thomas Lively via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:649
+    bool MustTail = (CallConv == CallingConv::Fast && CLI.IsTailCall &&
+                     MF.getTarget().Options.GuaranteedTailCallOpt) ||
+                    (CLI.CS && CLI.CS.isMustTailCall());
----------------
aheejin wrote:
> It's preexisting, but `GuaranteedTailCallOpt`'s description says:
> > When the flag is on, participating targets will perform tail call optimization on all calls which use the fastcc calling convention and which satisfy certain target-independent criteria (being at the end of a function, having the same return type as their parent function, etc.), using an alternate ABI if necessary. 
> 
> Does this mean, should we at least try to do tail calls even if `CLI.IsTailCall` is not true, i.e., there is no `tailcall` property in the instruction of .ll file? If so, it seems the check for this condition should be outside of `if (CLI.IsTailCall)`. Let me know if I'm mistaken.
I don't believe this is necessary.  I checked and neither RISCV nor AArch64 try to do tail calls unless `CLI.IsTailCall` is true. There is some pass that runs that marks all possible tail calls as `tail`, so I guess in the common case at least it would be duplicating effort to check again so late in the backend.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:664
+      if (!TypesMatch) {
+        assert(!MustTail);
+        CLI.IsTailCall = false;
----------------
aheejin wrote:
> We don't call `fail` in this case?
It should be an LLVM IR-level validation error if the prototypes of callers and their musttail callees do no match, so we should never observe this condition. I will add a comment.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:672
+    }
   }
 
----------------
aheejin wrote:
> I'm unsure how the user interface should work. When should we error out and when should we silently switch to non-tail calls? I might be mistaken, but it looks to me that unless `CLI.CS.isMustTailCall()` is true we don't need to error out, no? But currently `MustTail` contains much broader conditions, so I'm wondering what the UI should look like.
RISCV and AArch only error out when `CLI.CS.isMustTailCall()` is true, as you say. I will update this code to match that behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65246