Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix singlepass for Aarch64 #3415

Merged
merged 8 commits into from
Dec 9, 2022
Merged

Fix singlepass for Aarch64 #3415

merged 8 commits into from
Dec 9, 2022

Conversation

ptitSeb
Copy link
Contributor

@ptitSeb ptitSeb commented Dec 8, 2022

Description

Following debug of ticket #3343 , some issues in the singlepass were found.
In particular, some ADD/SUB opcode involving XSp with large value were not using the correct opcode, and sub xsp, xsp, x17 were encoded as neg xzr, x17 (a.k.a. sub xzr, xzr, x17). By forcing the dummy UXTX modifier, the correct opcode is selected.
Changed both SUB and ADD emiters.
Some alignement function specific to macOS ABI were also adjusted, and Local variable storage/retreival optimized a bit.

@ptitSeb ptitSeb requested a review from syrusakbary as a code owner December 8, 2022 13:01
@ptitSeb ptitSeb requested a review from theduke December 8, 2022 13:04
@ptitSeb ptitSeb added this to the v3.1 milestone Dec 8, 2022
Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test verifying the fix? (triggering what was failing)

Otherwise it looks good

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Dec 9, 2022

Can we add a test verifying the fix? (triggering what was failing)

Otherwise it looks good

I can try using the wasi test suite facility, that seems the more sane way to be sure to reproduce the issue (needs a .rs compile, so comparing between native / wasm as the wasi testdoes seems the best way).

Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Syrus said, a test would be great.
We should try to get into the habit of adding regression tests for each bug we fix.

Adding the Rust reproduction sounds good.

Would be nice to also have a minimal .wat test case, but probably not worth spending extra time on that if it's not trivial to recreate.

In the future we might also want to have some (singlepass) codegen snapshot tests.
As in: test that a specific input compiles to the expected Assembly.

lib/compiler-singlepass/src/emitter_arm64.rs Show resolved Hide resolved
@ptitSeb
Copy link
Contributor Author

ptitSeb commented Dec 9, 2022

As Syrus said, a test would be great. We should try to get into the habit of adding regression tests for each bug we fix.

Adding the Rust reproduction sounds good.

Would be nice to also have a minimal .wat test case, but probably not worth spending extra time on that if it's not trivial to recreate.

In the future we might also want to have some (singlepass) codegen snapshot tests. As in: test that a specific input compiles to the expected Assembly.

Yeah, well.
I have already spent 6h trying to get a "working" minimal test. A test for an architecture (Aarch64) that is NOT tested on the CI.
I'll add the test, but I'm not convinced this is good investment of time.

@theduke
Copy link
Contributor

theduke commented Dec 9, 2022

👍

I actually also wanted to mention above: we really need to get M1 CI up and running.

@ptitSeb
Copy link
Contributor Author

ptitSeb commented Dec 9, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 9, 2022

Build succeeded:

@bors bors bot merged commit 2d17aa9 into master Dec 9, 2022
@bors bors bot deleted the fix_singlepass_macm1 branch December 9, 2022 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants