Wasmtime GitHub notifications bot (Jun 27 2023 at 17:07): Wasmtime GitHub notifications bot (Jun 27 2023 at 17:07):alexcrichton commented on issue #6649:
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 an'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:
- Somehow get all wasm funtions to think they're using little-endian lane ordering. Previously this was done with the
Wasmtime*family of calling conventions but I'd ideally like to remove this Wasmtime "brand" from Cranelift to avoid all backends having to deal with it.- Converting between big and little-endian lane orders in the various trampolines as necessary. This may not actually be required since v128 is never passed in registers via
TypedFunc(haven't gotten around to implementing that in Wasmtime), so this point may already be "solved". In the future though I think it'd be fine to "just" generate more code in trampolines.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?
Wasmtime GitHub notifications bot (Jun 27 2023 at 17:23):fitzgen commented on issue #6649:
I think this will be easier once we switch all internal Wasm functions to using the
tailcalling convention, and then thetailcalling convention can specify a little-endian lane ordering.
Wasmtime GitHub notifications bot (Jun 27 2023 at 18:17):alexcrichton edited a comment on issue #6649:
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:
- Somehow get all wasm funtions to think they're using little-endian lane ordering. Previously this was done with the
Wasmtime*family of calling conventions but I'd ideally like to remove this Wasmtime "brand" from Cranelift to avoid all backends having to deal with it.- Converting between big and little-endian lane orders in the various trampolines as necessary. This may not actually be required since v128 is never passed in registers via
TypedFunc(haven't gotten around to implementing that in Wasmtime), so this point may already be "solved". In the future though I think it'd be fine to "just" generate more code in trampolines.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?
Wasmtime GitHub notifications bot (Jun 27 2023 at 18:29):alexcrichton commented on issue #6649:
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
tailif possible so I've leftWasmtimeSystemVfor now for s390x but left all the other changes so multi-return is not different in the calling convention and instead multi-return is handled differently in the native ABI.
Wasmtime GitHub notifications bot (Jun 27 2023 at 19:32):alexcrichton commented on issue #6649:
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.
Wasmtime GitHub notifications bot (Jun 27 2023 at 21:45):github-actions[bot] commented on issue #6649:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This 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:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Jun 28 2023 at 17:44):github-actions[bot] commented on issue #6649:
Subscribe to Label Action
cc @saulecabrera
<details>
This issue or pull request has been labeled: "winch"Thus the following users have been cc'd because of the following labels:
- saulecabrera: winch
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Last updated: Nov 22 2024 at 16:03 UTC