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

Winch aarch64 jmp #9051

Merged
merged 5 commits into from
Aug 1, 2024
Merged

Winch aarch64 jmp #9051

merged 5 commits into from
Aug 1, 2024

Conversation

vulc41n
Copy link
Contributor

@vulc41n vulc41n commented Jul 31, 2024

Hey 👋

This PR implements jmp, jmp_table and branch instructions for winch targeting aarch64.

#8321

@vulc41n vulc41n requested review from a team as code owners July 31, 2024 14:49
@vulc41n vulc41n requested review from cfallin and fitzgen and removed request for a team July 31, 2024 14:49
@vulc41n vulc41n force-pushed the winch-aarch64-jmp branch from 914588f to 0492dc3 Compare July 31, 2024 14:59
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Thanks! I think we need to add a little logic to handle large branch offsets and islands here (aarch64-specific issue, x64 doesn't need this) but otherwise LGTM.

tmp1: Reg,
tmp2: Reg,
) {
self.emit(Inst::JTSequence {
Copy link
Member

Choose a reason for hiding this comment

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

We'll need an EmitIsland instruction here too; see here and here for more details on how and why.

@github-actions github-actions bot added the winch Winch issues or pull requests label Jul 31, 2024
Copy link

Subscribe to Label Action

cc @saulecabrera

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 .github/subscribe-to-label.json configuration file.

Learn more.

@vulc41n
Copy link
Contributor Author

vulc41n commented Aug 1, 2024

Thanks for reviewing this work 🙂

Should I push the tests ? They are obviously large (13 and 12Mb).

Please tell me if there is anything else that I can improve

@cfallin
Copy link
Member

cfallin commented Aug 1, 2024

Should I push the tests ? They are obviously large (13 and 12Mb).

We have tests of the underlying parts (MachBuffer and its island-insertion logic); that is probably fine for coverage here, vs. checking in a very large test, IMHO. If we wanted to do better in the future we could add the equivalent of Cranelift's chaos mode choice to sometimes artificially lower the island threshold, to avoid the need for very large inputs to trigger this; but no need to do that in this PR.

@cfallin cfallin added this pull request to the merge queue Aug 1, 2024
Merged via the queue into bytecodealliance:main with commit c682d23 Aug 1, 2024
38 checks passed
@vulc41n vulc41n deleted the winch-aarch64-jmp branch August 1, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
winch Winch issues or pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants