Enable f16 tests on platforms that were missing conversion symbols
#129385
+4
−12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. [1]: rust-lang#125016
|
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
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 22, 2024
…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). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.try-job: dist-powerpc64-linux
try-job: dist-powerpc64le-linux
try-job: dist-riscv64-linux
try-job: dist-aarch64-msvc