Page MenuHomePhabricator

[WebAssembly] Support swiftself and swifterror for WebAssembly target
ClosedPublic

Authored by kateinoigakukun on Mar 12 2020, 1:14 AM.

Details

Summary

Swift ABI is based on basic C ABI described here https://github.com/WebAssembly/tool-conventions/blob/master/BasicCABI.md
Swift Calling Convention on WebAssembly is a little deffer from swiftcc
on another architectures.

On non WebAssembly arch, swiftcc accepts extra parameters that are
attributed with swifterror or swiftself by caller. Even if callee
doesn't have these parameters, the invocation succeed ignoring extra
parameters.

But WebAssembly strictly checks that callee and caller signatures are
same. https://github.com/WebAssembly/design/blob/master/Semantics.md#calls
So at WebAssembly level, all swiftcc functions end up extra arguments
and all function definitions and invocations explicitly have additional
parameters to fill swifterror and swiftself.

This patch support signature difference for swiftself and swifterror cc
is swiftcc.

e.g.

declare swiftcc void @foo(i32, i32)
@data = global i8* bitcast (void (i32, i32)* @foo to i8*)
define swiftcc void @bar() {
  %1 = load i8*, i8** @data
  %2 = bitcast i8* %1 to void (i32, i32, i32)*
  call swiftcc void %2(i32 1, i32 2, i32 swiftself 3)
  ret void
}

For swiftcc, emit additional swiftself and swifterror parameters
if there aren't while lowering. These additional parameters are added
for both callee and caller.
They are necessary to match callee and caller signature for direct and
indirect function call.

Diff Detail

Event Timeline

can you please upload this with full context (e.g. diff -U 999 or just use arcanist)?

kateinoigakukun edited the summary of this revision. (Show Details)

Upload diff using arc

Fix english syntax

I assume there's some kind of ABI doc that says how swiftself and swifterror are supposed to work and be lowered, is there one other than just the langref?

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
296

Is the idea here that some functions will have explicit swiftself/swifterror arguments in the IR and others won't (in which case, we create them when lowering)? Why the difference?

299

would it make sense to move all of this logic directly into computeSignatureVTs? It's repeated below and I think always right after a call to that.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
819

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?

kateinoigakukun marked 2 inline comments as done.Mar 13 2020, 7:11 PM

Here is Swift calling convention docs and there is description about swiftself and swifterror.

https://llvm.org/docs/LangRef.html#parameter-attributes
https://github.com/apple/swift/blob/master/docs/ABI/CallingConvention.rst#self

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
296

When the function doesn't have swiftself and swifterror, it can be called without swiftself and swifterror arguments directly.
swiftcc allows the number of arguments variance only when calling the function through function pointer, it's not allowed for direct function call. (As far as I know, this is helpful for static analysis to make it easier to inline or optimize)

299

I think it makes sense!

Harbormaster completed remote builds in B49205: Diff 250341.

I think the full diff still has not been uploaded yet; everywhere says "Context not available". Could you use arc diff to upload the diff?

llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
245 ↗(On Diff #250341)

typo: parameter

I think the full diff still has not been uploaded yet; everywhere says "Context not available". Could you use arc diff to upload the diff?

Hmm, I couldn't find "context not available" message... I updated diff using arc diff --update D76049. If there is still problem, I will re-open revision.

dschuff added inline comments.Mar 17 2020, 10:30 AM
llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
247 ↗(On Diff #250526)

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?

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
819

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?

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.h
162–165

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.

llvm/test/CodeGen/WebAssembly/swiftcc.ll
18

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

the diff context looks fine to me now, FWIW

kateinoigakukun marked 4 inline comments as done.

Remove unnecessary logic around WebAssemblyExplicitLocals and added some test cases

kateinoigakukun updated this revision to Diff 250962.EditedMar 17 2020, 6:58 PM
This comment has been deleted.
llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
247 ↗(On Diff #250526)

Oops, it's not necessary! Thanks

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
819

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.

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.h
162–165

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.

llvm/test/CodeGen/WebAssembly/swiftcc.ll
18

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.

I think the way you have it written here, it means that *all* swiftcc functions will end up with the extra parameters at the wasm level, and direct calls will also have the extra arguments. I think the test should also cover that (not just indirect calls), and I think the calling convention should be documented since it is basically the core of the swift ABI (even if you don't want to consider it stable yet). So at minimum there should be more explanation in the commit message and there should probably be a doc about the Swift ABI in https://github.com/WebAssembly/tool-conventions/ like there is for C.

Fortunately for functions that are not public or exported, I believe Binaryen should be able to optimize away the extra arguments after linking, so if you're not using it in your toolchain yet, you should.

kateinoigakukun edited the summary of this revision. (Show Details)Mar 19 2020, 2:28 AM
kateinoigakukun edited the summary of this revision. (Show Details)

Add test cases for direct function call and add more explanation

I think the calling convention should be documented since it is basically the core of the swift ABI (even if you don't want to consider it stable yet). So at minimum there should be more explanation in the commit message and there should probably be a doc about the Swift ABI in https://github.com/WebAssembly/tool-conventions/ like there is for C.

Right, I updated the description of this patch for more explanation and I'll create doc about Swift ABI on WebAssembly in https://github.com/WebAssembly/tool-conventions.

Fortunately for functions that are not public or exported, I believe Binaryen should be able to optimize away the extra arguments after linking, so if you're not using it in your toolchain yet, you should.

Thanks for your information! I'll try it.

dschuff accepted this revision.Mar 19 2020, 5:19 PM

Thanks!

This revision is now accepted and ready to land.Mar 19 2020, 5:19 PM

Thanks for reviewing! Can you merge this patch?

This revision was automatically updated to reflect the committed changes.