-
Notifications
You must be signed in to change notification settings - Fork 69
Use 64-bit words on wasm targets #973
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
Conversation
wasm is a 64-bit architecture with a 32-bit address space. We should be able to use 64-bit words for it without issue. I have not reviewed the u128 codegen to confirm it's non-branching on wasm, yet we already assume u128 codegen is non-branching for 64-bit platforms.
https://godbolt.org/z/bT86GeKnj appears to confirm I used |
|
||
#[inline] | ||
#[cfg(target_pointer_width = "32")] | ||
#[cfg(all(target_pointer_width = "32", not(target_family = "wasm")))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's some way to make an alias for that so that we could only spell out this condition once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also considered it, but I believe it'd require adding a build script to set a cfg for the desired word size. I didn't want to take that step when we have this single exception at this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was intending to solve this with the vaporwarecpubits
crate (which as it were uses a build script to set a cfg
attribute)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to take that step when we have this single exception at this time.
IMO this isn’t worth doing in a way that can’t be applied easily and in a DRY manner for multiple targets.
As noted in RustCrypto/utils#824 this same sort of 64-bit promotion should be applied to ARMv7 targets
Hm. godbolt claims this generates |
For |
This property applies to many targets, not just WASM, and trying to solve it this way is neither DRY nor scalable to many targets. As I just noted, I had intended to solve this problem with the |
See also: RustCrypto/utils#824 |
I’ll also note we have downstream crates that consume So really a PR like this also needs a companion PR to e.g. https://github.com/RustCrypto/elliptic-curves which will make clear this already un-DRY approach is impractical. That’s why I wanted to have one crate in charge of the selection that every other crate can easily consume. |
Ah, apologies. I was unaware of the prior art in those regards. I've just known this is an inefficiency for a while and wanted to finally improve it. A unified crate to yield this value would definitely be better. Re: elliptic-curves, wouldn't the better solution be to always use Feel free to close this PR if it's unhelpful though. |
@kayabaNerve in I do intend to support using |
Closing this, but if you’d like to help get |
wasm is a 64-bit architecture with a 32-bit address space. We should be able to use 64-bit words for it without issue. I have not reviewed the u128 codegen to confirm it's non-branching on wasm, yet we already assume u128 codegen is non-branching for 64-bit platforms.
https://users.rust-lang.org/t/are-there-any-negative-implications-of-using-u64-in-wasm32/57419 seems to casually discuss the premise of 64-bit integers within WASM decently.