[PATCH] D76049: [WebAssembly] Support swiftself and swifterror for WebAssembly target

[Yuta Saito via Phabricator via llvm-commits]

kateinoigakukun added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp:247
+  // if there aren't. Forward the cursor for the extra parameters.
+  if (MF.getFunction().getCallingConv() == CallingConv::Swift) {
+    bool HasSwiftErrorArg = false;
----------------
dschuff wrote:
> Is this bit actually necessary? in your change to `WebAssemblyTargetLowering::LowerFormalArguments` you do `MFI->addParam()` for the extra prams, so this should already be reflected in the value of `CurLocal` at this point? 
Oops, it's not necessary! Thanks


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:815
+      CLI.Outs.push_back(Arg);
+      SDValue ArgVal = DAG.getUNDEF(PtrVT);
+      CLI.OutVals.push_back(ArgVal);
----------------
dschuff wrote:
> dschuff wrote:
> > I assume this will actually be hooked up to some value instead of being undef? Where should these values actually come from? Are all functions expected to have these params?
> OK, I think i understand now. This is for when the callee takes swiftself/swifterror but the caller is not going to pass real values. What if the callee actually needs the self parameter though?

Now all swiftcc functions require swiftself and swifterror arguments, so we need to pass undef to fill the arguments. This is necessary to allow casting `void ()` to `void (swiftself i32, swifterror i32)`.

On the other hand, casting `void (swiftself i32, swifterror i32)` to `void ()` is not allowed, so it's OK to ignore the case "callee actually needs the self parameter though when caller is not going to pass real values".

When a function doesn’t have swiftself and swifterror in LLVM IR level, it always ignores the two extra parameters.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.h:163
 // signature for F (F is just used to get varous context)
-void computeSignatureVTs(const FunctionType *Ty, const Function &F,
-                         const TargetMachine &TM, SmallVectorImpl<MVT> &Params,
+void computeSignatureVTs(const FunctionType *Ty, const Function *TargetFunc,
+                         const Function &ContextFunc, const TargetMachine &TM,
----------------
dschuff wrote:
> Since we are now passing a pointer to `TargetFunc` directly, can we avoid also passing `Ty` (and just get the FunctionType inside computeSignatureVTs)?
> Please also update the comment.
`TargetFunc` can be `nullptr` because `GlobalValue` doesn’t always have a reference to real function. It can be a reference to another global variable. for example here llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:60

But the root global value which is referred from another global value, must have reference to real function, so the function type is already fixed.


================
Comment at: llvm/test/CodeGen/WebAssembly/swiftcc.ll:17
+; CHECK: call_indirect   (i32, i32, i32, i32) -> ()
+define swiftcc void @bar() {
+  %1 = load i8*, i8** @data
----------------
dschuff wrote:
> There should also be tests for casting in both directions (e.g. from a function with declared swiftself to one without and vice versa), and at least one where swifterror is used instead of swiftself. (e.g. try to cover all the new cases you added).
I added only the latter test cases. The former is not allowed in swiftcc. 
In swiftcc, casting `void (swiftself i32)`  to `void ()` is not allowed, swifterror also as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76049