Skip to content

Winch aarch64 jmp#9051

Merged
cfallin merged 5 commits intobytecodealliance:mainfrom
vulc41n:winch-aarch64-jmp
Aug 1, 2024
Merged

Winch aarch64 jmp#9051
cfallin merged 5 commits intobytecodealliance:mainfrom
vulc41n:winch-aarch64-jmp

Conversation

@vulc41n
Copy link
Copy Markdown
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
cfallin
cfallin reviewed Jul 31, 2024
View reviewed changes
Copy link
Copy Markdown
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.

winch/codegen/src/isa/aarch64/asm.rs Outdated
tmp1: Reg,
tmp2: Reg,
) {
self.emit(Inst::JTSequence {
Copy link
Copy Markdown
Member

@cfallin cfallin Jul 31, 2024

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
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 31, 2024

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

Learn more.

@vulc41n
Copy link
Copy Markdown
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
Copy Markdown
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
@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

Reviewers

@cfallin cfallin cfallin approved these changes

@fitzgen fitzgen Awaiting requested review from fitzgen fitzgen is a code owner automatically assigned from bytecodealliance/wasmtime-core-reviewers

Assignees

No one assigned

Labels

winch Winch issues or pull requests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vulc41n @cfallin