Enable f16 tests on platforms that were missing conversion symbols
#129385
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
S-waiting-on-review
|
@bors try |
|
This affects a ton of targets, I can almost guarantee at least one failure when we go to merge. @bors rollup=never |
tgross35
changed the title
f16 on platforms that were missing conversion symbolsf16 tests on platforms that were missing conversion symbols
…r=<try> Enable `f16` tests on platforms that were missing conversion symbols The only requirement for `f16` support, aside from LLVM not crashing and no ABI issues, is that symbols to convert to and from `f32` are available. Since the update to compiler-builtins in rust-lang#125016, we now provide these on all platforms. This also enables `f16` math since there are no further requirements. try-job: dist-powerpc64-linux try-job: dist-powerpc64le-linux try-job: dist-riscv64-linux try-job: dist-aarch64-msvc
|
I need to disable wasm since it is no longer caught by the default case this should match, https://github.com/rust-lang/compiler-builtins/blob/d96d3a7ca0e63fe990e2dc200f4c275591fa63f0/configure.rs#L62-L77. Will wait for this set of tests to complete. @beetrees mind checking my logic here? |
|
☀️ Try build successful - checks-actions |
|
While the symbols are now available on most targets, |
|
Ah that's right, I was just going off of the comments and forgot there is more to the story. I'll have to update jobs.yaml so these actually get verified in CI too. At least we should be able to enable Windows and MacOS. @rustbot author |
rustbot
added
S-waiting-on-author
|
@tgross35 Thanks for the ping! I did some quick testing and things seems to work as they should after I cherry-picked your commit. Maybe you should also remove the FIXMEs I added? Click here to see a diff of what I meanFrom 34ef3b44cd49a5a9081c522d9373f0f95a1803c8 Mon Sep 17 00:00:00 2001
From: Martin Nordholts <martn@axis.com>
Date: Thu, 22 Aug 2024 12:03:04 +0200
Subject: [PATCH] core: Enable f16 doctests for all linux archs
---
library/core/src/num/f16.rs | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/library/core/src/num/f16.rs b/library/core/src/num/f16.rs
index 41bd34a2702..64484d432a7 100644
--- a/library/core/src/num/f16.rs
+++ b/library/core/src/num/f16.rs
@@ -630,8 +630,7 @@ pub fn next_down(self) -> Self {
///
/// ```
/// #![feature(f16)]
- /// # // FIXME(f16_f128): extendhfsf2, truncsfhf2, __gnu_h2f_ieee, __gnu_f2h_ieee missing for many platforms
- /// # #[cfg(all(target_arch = "x86_64", target_os = "linux"))] {
+ /// # #[cfg(all(target_os = "linux"))] {
///
/// let x = 2.0_f16;
/// let abs_difference = (x.recip() - (1.0 / x)).abs();
@@ -650,8 +649,7 @@ pub fn recip(self) -> Self {
///
/// ```
/// #![feature(f16)]
- /// # // FIXME(f16_f128): extendhfsf2, truncsfhf2, __gnu_h2f_ieee, __gnu_f2h_ieee missing for many platforms
- /// # #[cfg(all(target_arch = "x86_64", target_os = "linux"))] {
+ /// # #[cfg(all(target_os = "linux"))] {
///
/// let angle = std::f16::consts::PI;
///
@@ -672,8 +670,7 @@ pub fn to_degrees(self) -> Self {
///
/// ```
/// #![feature(f16)]
- /// # // FIXME(f16_f128): extendhfsf2, truncsfhf2, __gnu_h2f_ieee, __gnu_f2h_ieee missing for many platforms
- /// # #[cfg(all(target_arch = "x86_64", target_os = "linux"))] {
+ /// # #[cfg(all(target_os = "linux"))] {
///
/// let angle = 180.0f16;
///
@@ -1175,8 +1172,7 @@ pub const fn from_bits(v: u16) -> Self {
///
/// ```
/// #![feature(f16)]
- /// # // FIXME(f16_f128): extendhfsf2, truncsfhf2, __gnu_h2f_ieee, __gnu_f2h_ieee missing for many platforms
- /// # #[cfg(all(target_arch = "x86_64", target_os = "linux"))] {
+ /// # #[cfg(all(target_os = "linux"))] {
///
/// struct GoodBoy {
/// name: &'static str,
--
2.39.2
|
|
@martn3 thanks for checking. Do you mean things build correctly, or tests actually pass? Based on @beetrees comment I have to expect that some things will fail, hence I'm going to back the MIPS changes (and others) out of this PR. Good point, the FIXMEs are fixed. I'm going to leave the gates x86-only for now until we know they pass on all platforms, I just don't want to list every arch or duplicate this config in |
|
@tgross35 f16 tests both build and pass for me. I run TEST_DEVICE_ADDR=192.168.0.173:12345 CARGOFLAGS_NOT_BOOTSTRAP=-Zdoctest-xcompile ./x.py test --set rust.verbose-tests=true --target mipsel-unknown-linux-gnu library/core library/std -- --test f16and get click to expand |
|
Huh. When cross-testing via QEMU with Not sure why we're getting different results. @martn3 What's your |
|
@beetrees My linker and MIPS system are not vanilla Debian, and I use special compiler and link flags, for example My config.toml is just |
|
That would explain it: llvm/llvm-project#97981 only occurs on hard-float MIPS (soft-float MIPS is only affected by llvm/llvm-project#97975). |
|
@tgross35 |
The only requirement for `f16` support, aside from LLVM not crashing and no ABI issues, is that symbols to convert to and from `f32` are available. Since the update to compiler-builtins in [1], we now provide these on all platforms. This also enables `f16` math since there are no further requirements. Still excluded are platforms for which LLVM emits infinitely-recursing code. [1]: rust-lang#125016
tgross35
force-pushed
the
more-platforms-enable-f16
branch
from
b557938
to
404c5d2
Compare
|
I updated this PR to reflect @beetrees comment in #129385 (comment). The only net change here is that @bors try |
…r=<try> Enable `f16` tests on platforms that were missing conversion symbols The only requirement for `f16` support, aside from LLVM not crashing and no ABI issues, is that symbols to convert to and from `f32` are available. Since the update to compiler-builtins in rust-lang#125016, we now provide these on all platforms. This also enables `f16` math since there are no further requirements. try-job: arm-android try-job: test-various try-job: x86_64-fuchsia
|
☀️ Try build successful - checks-actions |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (ed04567): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 2.2%, secondary 2.0%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 769.092s -> 767.6s (-0.19%) |
The only requirement for
f16support, aside from LLVM not crashing and no ABI issues, is that symbols to convert to and fromf32are available. Since the update to compiler-builtins in #125016, we now provide these on all platforms.This also enables
f16math since there are no further requirements.Still excluded are platforms for which LLVM emits infinitely-recursing code.
try-job: arm-android
try-job: test-various
try-job: x86_64-fuchsia