Skip to content
New issue

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

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

Already on GitHub? # to your account

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

Closed
saulecabrera opened this issue Dec 9, 2024 · 4 comments · Fixed by #9850
Closed

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

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

Comments

@saulecabrera
Copy link
Member

saulecabrera commented Dec 9, 2024

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
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.

@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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
winch Winch issues or pull requests
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants