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

Add numeric instructions for Wasm not available in core #1677

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Nov 17, 2024

The recently added wasm32v1-none target comes without std. This has caused some f32 and f64 methods to not be available for this target:

However, all these methods are available in Wasm via instructions that are part of Wasm v1. This PR exposes these methods as platform specific intrinsics.

It should be noted that there is a tracking issue (rust-lang/rust#50145) for bringing f32/f64::sqrt() to core, which would make adding this instruction obsolete.

I also made these functions insta-stable, is that alright? Should I make an ACP first? I'm also targetting v1.84, the release of the new wasm32v1-none target. Considering that the fork date is the 22nd, its unlikely this will make it in time.

Cc @alexcrichton.

@rustbot
Copy link
Collaborator

rustbot commented Nov 17, 2024

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@alexcrichton
Copy link
Member

Oh nice! Seems reasonable to me :)

I thought I had also seen some chatter on the Rust Zulip about making more floating-point methods available in libcore, so it might be good to sync up on that perhaps? I think it might also be best to start these out as unstable and then perhaps quickly follow up with a stabilization request with the rationale outlined?

For code changes, perhaps the documentation of one of these functions could explain why it's available as an intrinsic and why you might not want to use it most of the time (e.g. the float methods all do the same thing). Other methods could then all link to that one instance of the documentation to avoid duplicating everything here.

@daxpedda daxpedda force-pushed the wasm-numeric-instr branch 2 times, most recently from 5c27494 to 59bf920 Compare November 19, 2024 07:44
@daxpedda
Copy link
Contributor Author

I thought I had also seen some chatter on the Rust Zulip about making more floating-point methods available in libcore, so it might be good to sync up on that perhaps?

Couldn't find anything related on the quick, would appreciate a link if you have it on hand.

I think it might also be best to start these out as unstable and then perhaps quickly follow up with a stabilization request with the rationale outlined?

Sounds good, marked as unstable.

For code changes, perhaps the documentation of one of these functions could explain why it's available as an intrinsic and why you might not want to use it most of the time (e.g. the float methods all do the same thing). Other methods could then all link to that one instance of the documentation to avoid duplicating everything here.

I just added a one-liner to each function and linked to the appropriate method in std.

@alexcrichton
Copy link
Member

Took some digging as I also forgot, but I found:

I don't think that should hold this up personally though, but probably is worth considering for stabilization

@daxpedda
Copy link
Contributor Author

Ah, thanks. I was already aware of those and left a note in the OP.
AFAIU, taking into account our list of intrinsics, only f32/f64::sqrt() is still being considered.

@daxpedda
Copy link
Contributor Author

CI seems to be unblocked again.
@Amanieu this should be ready to go now.

@Amanieu
Copy link
Member

Amanieu commented Nov 29, 2024

This seems fine to add on nightly, but I wouldn't expect these to be actually stabilized: I would much prefer if we could have this functionality work directly on floats, even with #[no_std].

If you still think this is worth it, please create a tracking issue and add the number the unstable attributes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants