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.

Sign up for GitHub

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

Jump to bottom

winch(aarch64): Improve 32-bit {s,u}div #9766

Open
saulecabrera opened this issue Dec 9, 2024 · 4 comments · May be fixed by #9850
Open

winch(aarch64): Improve 32-bit {s,u}div #9766

saulecabrera opened this issue Dec 9, 2024 · 4 comments · May be fixed by #9850
Labels
winch Winch issues or pull requests

Comments

@saulecabrera
Copy link
Member

saulecabrera commented Dec 9, 2024 •
edited
Loading

While reviewing #9762, I realized that in the case of 32-bit operands a sign-extension is required given that cranelift-codegen, only accepts 64-bit operands.

The ARM64 documentation states 1, 2 that 32-bit operands for division are allowed.

Introducing support for 32-bit operands means that in Winch we can skip the extension sequence when dealing with 32-bit operands.

As a side note, it seems that when migrating to ISLE the intention was to add support for 32-bit operands.

cc @alexcrichton / @cfallin I'm not entirely familiar with the development history of the Aarch64 backend, is there any other reason to be aware of when considering support for 32-bit operands?
@saulecabrera saulecabrera converted this from a draft issue Dec 9, 2024
@saulecabrera saulecabrera moved this to Todo in Winch Dec 9, 2024
@saulecabrera saulecabrera changed the title winch: Improve 32-bit {s,u}div winch(aarch64): Improve 32-bit {s,u}div Dec 9, 2024
@cfallin
Copy link
Member

cfallin commented Dec 9, 2024

I'm not aware of any reasons why we couldn't add 32-bit variants of the div instruction family; going all the way back to the original PR for the aarch64 backend it looks like I had only implemented SDiv64 and UDiv64 and added the extensions. Probably this is a case of "no one has had occasion to add it yet"...

(Incidentally, x86-64 has 32-bit divides too; and on both architectures I'd expect that high-performance implementations would probably take ~half the latency for a 32-bit version; so not a small win for any benchmark with a divide in a critical path.)

@saulecabrera saulecabrera added the winch Winch issues or pull requests label Dec 9, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 9, 2024

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.

@saulecabrera saulecabrera mentioned this issue Dec 9, 2024
Merged
@MarinPostma
Copy link
Contributor

I'd like to take a stab at that after I'm done with winch. Any guidance on how I should tackle that?

@saulecabrera
Copy link
Member Author

One approach could be to take a look at how similar instructions have been added in the past e.g., #8453

This was referenced Dec 12, 2024
Merged
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
winch Winch issues or pull requests
Projects
Winch
Status: Todo
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cranelift: 32bit div_s, rem_u, rem_s for aarch64 MarinPostma/wasmtime
3 participants
@cfallin @saulecabrera @MarinPostma