Wasmtime GitHub notifications bot (Dec 06 2024 at 15:25): Wasmtime GitHub notifications bot (Dec 06 2024 at 15:25):MarinPostma opened PR #9751 from MarinPostma:aarch64-call to bytecodealliance:main:
Implement the following MASM instruction for the aarch64 platform:
- call
- address_at_vmctx
- trapz
I initially planned to only implement call, but implementing the tests uncovered the necessity to implement the other two as prerequisite for indirect calls.
Right now the calling convention is hardcoded to SystemV, but maybe we also want to support AppleAarch64?
#8321
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Dec 06 2024 at 15:25):MarinPostma requested wasmtime-compiler-reviewers for a review on PR #9751.
Wasmtime GitHub notifications bot (Dec 06 2024 at 15:25):MarinPostma requested abrown for a review on PR #9751.
Wasmtime GitHub notifications bot (Dec 06 2024 at 15:25):MarinPostma requested fitzgen for a review on PR #9751.
Wasmtime GitHub notifications bot (Dec 06 2024 at 15:25):MarinPostma requested wasmtime-core-reviewers for a review on PR #9751.
Wasmtime GitHub notifications bot (Dec 06 2024 at 16:10):MarinPostma updated PR #9751.
Wasmtime GitHub notifications bot (Dec 06 2024 at 16:16):saulecabrera commented on PR #9751:
I can help taking a look at this one.
Wasmtime GitHub notifications bot (Dec 06 2024 at 16:16):saulecabrera requested saulecabrera for a review on PR #9751.
Wasmtime GitHub notifications bot (Dec 06 2024 at 16:47):github-actions[bot] commented on PR #9751:
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>
Wasmtime GitHub notifications bot (Dec 06 2024 at 19:10):saulecabrera submitted PR review:
Generally looks great, thanks. Left some inline nitpicks.
Wasmtime GitHub notifications bot (Dec 06 2024 at 19:10):saulecabrera created PR review comment:
Would you mind adding some comments to these methods?
Wasmtime GitHub notifications bot (Dec 06 2024 at 19:10):saulecabrera created PR review comment:
Right now the calling convention is hardcoded to SystemV, but maybe we also want to support AppleAarch64?
I see why this might be confusing. Even though the
CallConvis hard coded here, there are two things to note:
- All the infrastructure to handle the calling convention differences, is handled very early on in the compilation process in Winch (and in Cranelift as well) e.g., when constructing the signature for the callee, at that point, you'll note that Winch correctly handles the default and apple-aarch64 calling conventions.
- Winch relies on Cranelift's binary emission infrastructure, and in this concrete case, as you can note, you're constructing a
CallInfo::empty(), which signals that none of the other Cranelift-specific invariants must be taken into account when emitting calls (some of them depend on the calling convention that is passed here).I've opened https://github.com/bytecodealliance/wasmtime/pull/9757 to refactor call emission a bit with the hope of making this part more consistent.
Wasmtime GitHub notifications bot (Dec 06 2024 at 19:10):saulecabrera created PR review comment:
/// Trap if `rn` is zero.
Wasmtime GitHub notifications bot (Dec 06 2024 at 20:19):MarinPostma submitted PR review.
Wasmtime GitHub notifications bot (Dec 06 2024 at 20:19):MarinPostma created PR review comment:
That refactor makes a lot of sense to me :+1:
Wasmtime GitHub notifications bot (Dec 06 2024 at 20:28):MarinPostma updated PR #9751.
Wasmtime GitHub notifications bot (Dec 06 2024 at 20:28):MarinPostma submitted PR review.
Wasmtime GitHub notifications bot (Dec 06 2024 at 20:28):MarinPostma created PR review comment:
fixed in f1a56f1
Wasmtime GitHub notifications bot (Dec 06 2024 at 20:28):MarinPostma submitted PR review.
Wasmtime GitHub notifications bot (Dec 06 2024 at 20:28):MarinPostma created PR review comment:
fixed in f1a56f1
Wasmtime GitHub notifications bot (Dec 06 2024 at 20:29):MarinPostma submitted PR review.
Wasmtime GitHub notifications bot (Dec 06 2024 at 20:29):MarinPostma created PR review comment:
updated in f4a6cf7
Wasmtime GitHub notifications bot (Dec 06 2024 at 20:30):MarinPostma updated PR #9751.
Wasmtime GitHub notifications bot (Dec 06 2024 at 20:31):MarinPostma updated PR #9751.
Wasmtime GitHub notifications bot (Dec 06 2024 at 20:32):MarinPostma requested saulecabrera for a review on PR #9751.
Wasmtime GitHub notifications bot (Dec 06 2024 at 22:10):saulecabrera commented on PR #9751:
There are some failures in CI -- I'm trying to figure out if they are related to your changes or not.
Wasmtime GitHub notifications bot (Dec 06 2024 at 22:28):saulecabrera submitted PR review.
Wasmtime GitHub notifications bot (Dec 06 2024 at 22:28):saulecabrera created PR review comment:
I'm not entirely sure if this is the source of the failure, but here you need to specify
;;! test = "winch"Else this will use Cranelift
Wasmtime GitHub notifications bot (Dec 07 2024 at 00:28):MarinPostma updated PR #9751.
Wasmtime GitHub notifications bot (Dec 07 2024 at 00:55):MarinPostma submitted PR review.
Wasmtime GitHub notifications bot (Dec 07 2024 at 00:55):MarinPostma created PR review comment:
Some submodule update sneaked into one of my commits. That's what's messing with the CI, I'll fix that tommorow
Wasmtime GitHub notifications bot (Dec 07 2024 at 10:50):MarinPostma updated PR #9751.
Wasmtime GitHub notifications bot (Dec 07 2024 at 13:36):MarinPostma commented on PR #9751:
Alright, I updated the submodule, ci looks good now
Wasmtime GitHub notifications bot (Dec 09 2024 at 11:30):saulecabrera submitted PR review:
Thanks!
Wasmtime GitHub notifications bot (Dec 09 2024 at 11:47):saulecabrera merged PR #9751.
Last updated: Dec 23 2024 at 12:05 UTC