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

wasm32 and aarch64 intrinsics for rint and rintf #430

Merged
merged 2 commits into from
Jan 12, 2025

Conversation

hanna-kruppe
Copy link
Contributor

I didn't bother with x86 because SSE4.1-exclusive code paths are extra testing burden that's unlikely to benefit many people.

Closes #421

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record: there's a bunch of other libm functions that could be added here (sqrt, fma, probably floor/ceil/trunc as well) but I won't have time to work on that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually I will probably attempt to script together the operations from https://github.com/kraj/musl/tree/eb4309b142bb7a8bdc839ef1faf18811b9ff31c8/src/math/aarch64 (and the other architectures), most of the rounding-related ops are covered there. But yeah, it isn't a priority for the time being.

@tgross35
Copy link
Contributor

Thanks for jumping on this, the wasm changes look good to me. For aarch64, is there any reason not to just use assembly to get the non-vector version? For reference https://github.com/kraj/musl/blob/eb4309b142bb7a8bdc839ef1faf18811b9ff31c8/src/math/aarch64/rint.c

@tgross35
Copy link
Contributor

Also just as a note, wasm checks the build but doesn't actually run any tests. I am not sure of a good way to do so, but am open to ideas if you have any.

@hanna-kruppe
Copy link
Contributor Author

hanna-kruppe commented Jan 12, 2025

Thanks for jumping on this, the wasm changes look good to me. For aarch64, is there any reason not to just use assembly to get the non-vector version? For reference https://github.com/kraj/musl/blob/eb4309b142bb7a8bdc839ef1faf18811b9ff31c8/src/math/aarch64/rint.c

I don't think there's any fundamental obstacle (I though MSRV might be a problem but apparently not), I just don't fully trust myself to write that inline asm correctly and the only cost from using the vector variant is an extra vdup, which is very cheap. (I don't think the scalar vs. vector form makes any difference for the performance of rounding operation itself, at least as long as you stick to 64 bit vectors.)

$ cargo asm --target aarch64-unknown-linux-gnu --features unstable-intrinsics -p libm --lib rintf
.section .text.libm::math::rintf::rintf,"ax",@progbits
        .globl  libm::math::rintf::rintf
        .p2align        2
        .type   libm::math::rintf::rintf,@function
libm::math::rintf::rintf:
        .cfi_startproc
        dup v0.2s, v0.s[0]
        frintn v0.2s, v0.2s
        ret

Also just as a note, wasm checks the build but doesn't actually run any tests. I am not sure of a good way to do so, but am open to ideas if you have any.

You can run cargo test by installing wasmtime and setting target.<triple>.runner = "wasmtime" in the Cargo configuration (.cargo/config.toml or environment variable). I did this manually and it passed except for check_coverage::ensure_list_is_updated because it can't shell out. I don't know how this interacts with the other tests not run via cargo test.

@tgross35 tgross35 merged commit 67537a9 into rust-lang:master Jan 12, 2025
35 checks passed
@tgross35
Copy link
Contributor

I don't think there's any fundamental obstacle (I though MSRV might be a problem but apparently not), I just don't fully trust myself to write that inline asm correctly and the only cost from using the vector variant is an extra vdup, which is very cheap. (I don't think the scalar vs. vector form makes any difference for the performance of rounding operation itself, at least as long as you stick to 64 bit vectors.)

For anything where MSRV would be a problem, I have just been gating things under unstable_intrinsics so at least anybody using this via std will benefit. There are some things that could be more fine grained, but I don't think it's worth adding more to possible feature combinations.

You can run cargo test by installing wasmtime and setting target.<triple>.runner = "wasmtime" in the Cargo configuration (.cargo/config.toml or environment variable). I did this manually and it passed except for check_coverage::ensure_list_is_updated because it can't shell out. I don't know how this interacts with the other tests not run via cargo test.

I do need to document this better but the only tests enabled by default are a handful of spotchecks, it needs the build-musl feature to build musl and compare against that, and/or test-multiprecision to test against MPFR. I have my doubts that either of those are likely work on wasm, unfortunately.

Thanks for the additions here!

@hanna-kruppe
Copy link
Contributor Author

For what it's worth, wasi-libc uses a lightly modified musl for its libm and is used by Rust's wasm32-wasip1 target as part of std, so it might be relatively easy to compare against that. Though it might not be very useful for this specific change because they probably just end up using the same lowering for any functions that map directly to wasm instructions.

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.

Use core::arch intrinsics for for rint and rintf where possible
2 participants