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 fast type conversion methods where possible #117

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

shssoichiro
Copy link
Contributor

For SIMD types that have intrinsics for quickly
converting to another type, methods were added to
leverage these intrinsics for fast conversion.

For SIMD types that have intrinsics for quickly
converting to another type, methods were added to
leverage these intrinsics for fast conversion.
@shssoichiro
Copy link
Contributor Author

shssoichiro commented Oct 27, 2022

As someone who is not an expert on wasm, I have no idea why the wasm tests are failing saying that f32 and f64 do not have a round method. 😕

@Lokathor
Copy link
Owner

@CryZe you're the wasm person, shouldn't round be a normal method? or is this a no_std related thing maybe?

@CryZe
Copy link
Contributor

CryZe commented Oct 27, 2022

From a quick glance this looks like a no_std problem, as any libm call (round, sin, cos, ...) requires std or the usage of the libm crate.

@shssoichiro
Copy link
Contributor Author

shssoichiro commented Oct 27, 2022

Is there a recommended fix? The one that comes to mind for me is to only include the round methods if std is included.

Edit: I see that there's e.g. a f32x4::round() method although it claims that the fallback implementation is quite slow...

@Lokathor
Copy link
Owner

The fallback is to call libm and do it in hardware. However, that fallback ends up being called only with no_std outside of x86 or x86_64. So... maybe that's okay?

@Lokathor
Copy link
Owner

I guess for new methods, we should make the method not exist if it can't perform well. people can always turn on the std feature.

@shssoichiro
Copy link
Contributor Author

It probably makes sense to add this std fallback on round, so I posted #118

@shssoichiro
Copy link
Contributor Author

Likewise I fixed this PR up to use the f**x*::round functions, which will benefit from the patch in #118 once that's merged as well.

@shssoichiro
Copy link
Contributor Author

I'm also noticing that a few of these do have existing functions, for example f32x4::fast_round_int which I didn't see before because I was looking for the wrong thing probably. What should we do in those cases? Have f32x4::to_i32x4 call through to fast_round_int perhaps? (Even though this results in two methods that do the same thing, I personally like the idea of having consistent naming for all casting methods, so it's easy for users to find.)

@Lokathor
Copy link
Owner

I agree that naming consistency would be best.

Let's make the old names just call to the new name version, and then we can mark the old names as deprecated with a note that people should move to the new names instead. Then eventually we can remove the old names entirely in 0.8 or 0.9 or whenever.

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

Successfully merging this pull request may close these issues.

3 participants