winch: Implement check_stack for aarch64#10763
winch: Implement check_stack for aarch64#10763saulecabrera merged 7 commits intobytecodealliance:mainfrom
Conversation
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "winch"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
a1a87f5 to
8a8bccc
Compare
|
(moving review over to @saulecabrera) Looks like you found |
saulecabrera
left a comment
There was a problem hiding this comment.
This is looking really good, thanks! I left a couple of comments inline, which I think is probably worth addressing.
In terms of tests, since our last discussion via Zulip, #10750 landed, which enables running some aarch64 tests in CI. It's probably a good idea to enable at least the call.wast tests here https://github.com/bytecodealliance/wasmtime/blob/main/crates/test-util/src/wast.rs#L422. Ideally, we should enable all of them, but it's possible that some of them will fail, due to some of the existing bugs that I'm currently in the process of fixing.
|
@saulecabrera thanks for the review! one thing worth pointing out is that this change doesn't include the bit about emitting unwind instructions that's in the x86_64 masm. I got the impression that's a larger project though (there doesn't seem to be any unwind support in the aarch64 backend at all right now -- is that something else that needs doing?) |
|
@graydon You're right that there is no unwind support right now, it's not expected to land that functionality as part of this PR. I think that once that (any passing) tests are updated here https://github.com/bytecodealliance/wasmtime/blob/main/crates/test-util/src/wast.rs#L422, we should be able to land this one. |
d6fb23b to
08b2402
Compare
This is my first attempt at contributing to winch or cranelift at all -- wide open to any feedback/suggestions. I was just poking around looking for things I could do to help push winch over the finish line on aarch64. @saulecabrera suggested the missing
check_stackimplementation might be a welcome change.(This is also my first time working with aarch64 instructions in any detail; hopefully I got the semantics not-too-wrong)
Before:
After: