Remove Wasmtime ABIs from Cranelift #6649
Conversation
This commit removes the `Wasmtime*` family of ABIs from Cranelift. These were originally added to support multi-value in Wasmtime via the `TypedFunc` API, but they should now no longer be necessary. In general this is a higher-level Wasmtime concern than something all backends of Cranelift should have to deal with. Today with recent refactorings it's possible to remove the reliance on ABI details for multi-value and instead codify it directly into the Cranelift IR generated. For example wasm calls are able to have a "purely internal" ABI which Wasmtime's Rust code doesn't see at all, and the Rust code only interacts with the native ABI. The native ABI is redefined to be what the previous Wasmtime ABIs were, which is to return the first of a 2+ value return through a register (native return value) and everything else through a return pointer.
|
cc @uweigand if you're up for it I could use some help on the s390x-side of things. I believe that the Wasmtime ABI was load-bearing in a way not related to multi-value on the s390x backend to handle lane order of vector types, meaning that I think as-is this can't land since it'll fail tests on s390x. I was wondering if you had thoughts or ideas about how to go about fixing this? In my mind the fix would look something along the lines of:
So I guess the question boils down to: should an ABI be added for s390x to specify that the lane order of vectors is little-endian? Or are there other options you can think of to solve this? |
|
I think this will be easier once we switch all internal Wasm functions to using the |
|
Yeah the more I think about this the more I realize it's probably best to leave things as-is and tied to the calling convention for now. I think it'd be good nonetheless to get stuff in ahead of time before |
|
Hurray looks like tests are green on s390x! I think this is good to go and we can tackle how to avoid having a specific ABI for s390x, if at all, in the future. |
| // On S390x we need a Wasmtime calling convention to ensure | ||
| // we're using little-endian vector lane order. | ||
| wasmtime_call_conv(isa) | ||
| CallConv::AppleAarch64 |
There was a problem hiding this comment.
Do we always need something special here? Because I'd like this function to unconditionally use CallConv::Tail soon, but I don't understand the details of this apple aarch64 stuff.
There was a problem hiding this comment.
Hm so I'm not actually entirely sure what's going on here. This comment was added in #4195 with relation to pointer-authentication support on AArch64 I believe, but I'm not sure what ended up leading to the apple-aarch64 change here. I can confirm locally that if I change this to Fast that everything passes locally.
I'll file this as a follow-up to have it as a separate change.
github-actions
bot
added
cranelift
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "winch"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
* Remove Wasmtime ABIs from Cranelift This commit removes the `Wasmtime*` family of ABIs from Cranelift. These were originally added to support multi-value in Wasmtime via the `TypedFunc` API, but they should now no longer be necessary. In general this is a higher-level Wasmtime concern than something all backends of Cranelift should have to deal with. Today with recent refactorings it's possible to remove the reliance on ABI details for multi-value and instead codify it directly into the Cranelift IR generated. For example wasm calls are able to have a "purely internal" ABI which Wasmtime's Rust code doesn't see at all, and the Rust code only interacts with the native ABI. The native ABI is redefined to be what the previous Wasmtime ABIs were, which is to return the first of a 2+ value return through a register (native return value) and everything else through a return pointer. * Remove some wasmtime_system_v usage in tests * Add back WasmtimeSystemV for s390x * Fix some docs and references in winch
This resolves two issues from recent changes in bytecodealliance#6649: * First the s390x calling convention for wasm functions is changed back to `WasmtimeSystemV` from `Fast`. This was an accidental omission from bytecodealliance#6649 where the conclusion was that s390x will continue using a calling convention with little-endian lane order for lane arguments. The only calling convention that supports this today is `WasmtimeSystemV`, although the `Tail` calling convention will likely use it in the future as well. * Second the apple-aarch64 platform now uses the `Fast` calling convention instead of `AppleAarch64` calling convention. That convention was specified in bytecodealliance#4195 but local testing shows that is not necessary in the sense that tests all pass with the `Fast` calling convention. This means that the prior comment why the `AppleAarch64` calling convention is required is no longer accurate and in the absence of a reason not to I went ahead and switched it to `Fast`. In the near future all wasm functions will unconditionally use the `Tail` calling convention and at that time the lane order can be specified on s390x to be little-endian to satisfy all the constraints here. Additionally any unwinding directives, if necessary for aarch64, can be specified as needed.
This resolves two issues from recent changes in bytecodealliance#6649: * First the s390x calling convention for wasm functions is changed back to `WasmtimeSystemV` from `Fast`. This was an accidental omission from bytecodealliance#6649 where the conclusion was that s390x will continue using a calling convention with little-endian lane order for lane arguments. The only calling convention that supports this today is `WasmtimeSystemV`, although the `Tail` calling convention will likely use it in the future as well. * Second the apple-aarch64 platform now uses the `Fast` calling convention instead of `AppleAarch64` calling convention. That convention was specified in bytecodealliance#4195 but local testing shows that is not necessary in the sense that tests all pass with the `Fast` calling convention. This means that the prior comment why the `AppleAarch64` calling convention is required is no longer accurate and in the absence of a reason not to I went ahead and switched it to `Fast`. In the near future all wasm functions will unconditionally use the `Tail` calling convention and at that time the lane order can be specified on s390x to be little-endian to satisfy all the constraints here. Additionally any unwinding directives, if necessary for aarch64, can be specified as needed.
* Update calling conventions for wasm functions slightly This resolves two issues from recent changes in #6649: * First the s390x calling convention for wasm functions is changed back to `WasmtimeSystemV` from `Fast`. This was an accidental omission from #6649 where the conclusion was that s390x will continue using a calling convention with little-endian lane order for lane arguments. The only calling convention that supports this today is `WasmtimeSystemV`, although the `Tail` calling convention will likely use it in the future as well. * Second the apple-aarch64 platform now uses the `Fast` calling convention instead of `AppleAarch64` calling convention. That convention was specified in #4195 but local testing shows that is not necessary in the sense that tests all pass with the `Fast` calling convention. This means that the prior comment why the `AppleAarch64` calling convention is required is no longer accurate and in the absence of a reason not to I went ahead and switched it to `Fast`. In the near future all wasm functions will unconditionally use the `Tail` calling convention and at that time the lane order can be specified on s390x to be little-endian to satisfy all the constraints here. Additionally any unwinding directives, if necessary for aarch64, can be specified as needed. * Fix compile
2 participants
This commit removes the
Wasmtime*family of ABIs from Cranelift. These were originally added to support multi-value in Wasmtime via theTypedFuncAPI, but they should now no longer be necessary. In general this is a higher-level Wasmtime concern than something all backends of Cranelift should have to deal with.Today with recent refactorings it's possible to remove the reliance on ABI details for multi-value and instead codify it directly into the Cranelift IR generated. For example wasm calls are able to have a "purely internal" ABI which Wasmtime's Rust code doesn't see at all, and the Rust code only interacts with the native ABI. The native ABI is redefined to be what the previous Wasmtime ABIs were, which is to return the first of a 2+ value return through a register (native return value) and everything else through a return pointer.