Skip to content

tests: Also gate f16::erfc() doctest with reliable_f16_math cfg - #137167

Merged
bors merged 1 commit into
rust-lang:masterfrom
martn3:reliable_f16_math-f16-erfc
Feb 18, 2025
Merged

tests: Also gate f16::erfc() doctest with reliable_f16_math cfg#137167
bors merged 1 commit into
rust-lang:masterfrom
martn3:reliable_f16_math-f16-erfc

Conversation

@martn3

@martn3 martn3 commented Feb 17, 2025

Copy link
Copy Markdown

In #136324 the doctest for f16::erf() was gated with reliable_f16_math. Add the same gate on f16::erfc() to avoid:

rust_out.71e2e529d20ea47d-cgu.0:\
(.text._ZN8rust_out4main43_doctest_main_library_std_src_f16_rs_1321_017h485f3ffe6bf2a981E+0x38): \
undefined reference to `__gnu_h2f_ieee'

on MIPS (and maybe other architectures).

r? tgross35

@rustbot

rustbot commented Feb 17, 2025

Copy link
Copy Markdown
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 17, 2025
In 136324 the doctest for `f16::erf()` was gated with
`reliable_f16_math`. Add the same gate on `f16::erfc()` to
avoid:

    rust_out.71e2e529d20ea47d-cgu.0:\
    (.text._ZN8rust_out4main43_doctest_main_library_std_src_f16_rs_1321_017h485f3ffe6bf2a981E+0x38): \
    undefined reference to `__gnu_h2f_ieee'

on MIPS (and maybe other architectures).
@martn3
martn3 force-pushed the reliable_f16_math-f16-erfc branch from 799639b to f6485ff Compare February 17, 2025 10:59
@martn3 martn3 changed the title std: Also gate f16::erfc() with reliable_f16_math cfg tests: Also gate f16::erfc() doctest with reliable_f16_math cfg Feb 17, 2025

@GrigorenkoPV GrigorenkoPV left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, missed this one. I've double-checked now, and looks like this indeed is the only one that's missing.

@tgross35

Copy link
Copy Markdown
Contributor

@bors r+ rollup

@bors

bors commented Feb 17, 2025

Copy link
Copy Markdown
Collaborator

📌 Commit f6485ff has been approved by tgross35

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 17, 2025
@tgross35

Copy link
Copy Markdown
Contributor

Thanks for the catch, unfortunately I don't think we run tests on any platforms that don't support f16

rust/library/std/build.rs

Lines 97 to 118 in ce36a96

let has_reliable_f16 = match (target_arch.as_str(), target_os.as_str()) {
// We can always enable these in Miri as that is not affected by codegen bugs.
_ if is_miri => true,
// Selection failure <https://github.com/llvm/llvm-project/issues/50374>
("s390x", _) => false,
// Unsupported <https://github.com/llvm/llvm-project/issues/94434>
("arm64ec", _) => false,
// MinGW ABI bugs <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115054>
("x86_64", "windows") if target_env == "gnu" && target_abi != "llvm" => false,
// Infinite recursion <https://github.com/llvm/llvm-project/issues/97981>
("csky", _) => false,
("hexagon", _) => false,
("loongarch64", _) => false,
("mips" | "mips64" | "mips32r6" | "mips64r6", _) => false,
("powerpc" | "powerpc64", _) => false,
("sparc" | "sparc64", _) => false,
("wasm32" | "wasm64", _) => false,
// `f16` support only requires that symbols converting to and from `f32` are available. We
// provide these in `compiler-builtins`, so `f16` should be available on all platforms that
// do not have other ABI issues or LLVM crashes.
_ => true,
};
. s390x has a dist job but that doesn't run anything.

However, I think we could actually remove mips from that list if llvm/llvm-project#110199 made it into llvm 20, which Rust just upgraded to. @martn3 if that sounds right, you could try deleting https://github.com/rust-lang/compiler-builtins/blob/3c09866aa4c699ed5f430567f6de97ed56b65126/configure.rs#L81 and then running tests. If it works also update the above std/build.rs, and then we have f16 on mips 🎉

@bors
bors merged commit 14fb84a into rust-lang:master Feb 18, 2025
@rustbot rustbot added this to the 1.87.0 milestone Feb 18, 2025
@martn3

martn3 commented Feb 19, 2025

Copy link
Copy Markdown
Author

However, I think we could actually remove mips from that list if llvm/llvm-project#110199 made it into llvm 20, which Rust just upgraded to. @martn3 if that sounds right, you could try deleting https://github.com/rust-lang/compiler-builtins/blob/3c09866aa4c699ed5f430567f6de97ed56b65126/configure.rs#L81 and then running tests. If it works also update the above std/build.rs, and then we have f16 on mips 🎉

@tgross35 According to my testing, f16 works on MIPS with Rust master branch with LLVM 20 🎉 . I ran with Rust master plus this:

diff --git a/configure.rs b/configure.rs
index fa3e302..ff52e88 100644
--- a/configure.rs
+++ b/configure.rs
@@ -80,3 +80,2 @@ pub fn configure_f16_f128(target: &Target) {
         "loongarch64" => false,
-        "mips" | "mips64" | "mips32r6" | "mips64r6" => false,
         "powerpc" | "powerpc64" => false,

and this:

diff --git a/library/Cargo.lock b/library/Cargo.lock
index 0be2f9a1549..5340d74fc3c 100644
--- a/library/Cargo.lock
+++ b/library/Cargo.lock
@@ -64,4 +64,3 @@ name = "compiler_builtins"
 version = "0.1.146"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "a97117b1434b79833f39a5fabdf82f890bd98c1988334dea1cb67f7e627fa311"
+source = "git+https://github.com/martn3/compiler-builtins.git?branch=f16-mips#b15a085cf1ca4b8e9d31b56dd564bb8954306ae5"
 dependencies = [
diff --git a/library/Cargo.toml b/library/Cargo.toml
index 1205f7c9ed6..b973d04d99e 100644
--- a/library/Cargo.toml
+++ b/library/Cargo.toml
@@ -43,2 +43,3 @@ rustc-demangle.debug = 0
 [patch.crates-io]
+compiler_builtins = { git = 'https://github.com/martn3/compiler-builtins.git', branch = "f16-mips" }
 # See comments in `library/rustc-std-workspace-core/README.md` for what's going on
diff --git a/library/std/build.rs b/library/std/build.rs
index 8dc326a3dde..723d1eb02e0 100644
--- a/library/std/build.rs
+++ b/library/std/build.rs
@@ -109,3 +109,2 @@ fn main() {
         ("loongarch64", _) => false,
-        ("mips" | "mips64" | "mips32r6" | "mips64r6", _) => false,
         ("powerpc" | "powerpc64", _) => false,

Disclaimer: I only tested on the MIPS hardware that happens to be available to me, and can't say anything about other MIPS hardware.

Disclaimer: I didn't run tests that depend on conformant NaNs due to MIPS having non-conformat NaNs. In particular I skipped these tests: f16::f16::powf, f32::f32::powf, f64::f64::powf, SimdFloat::reduce_max, SimdFloat::reduce_min, iter::traits::iterator::Iterator::max, test_min_max_nan.

@tgross35

tgross35 commented Feb 19, 2025

Copy link
Copy Markdown
Contributor

That's great news! Feel free to put up PRs to make those changes and r? me.

Disclaimer: I didn't run tests that depend on conformant NaNs due to MIPS having non-conformat NaNs. In particular I skipped these tests: f16::f16::powf, f32::f32::powf, f64::f64::powf, SimdFloat::reduce_max, SimdFloat::reduce_min, iter::traits::iterator::Iterator::max, test_min_max_nan.

If those tests rely on total_cmp ordering between sNaNs and qNaNs, it may be feasible to adjust the implementations to flip the quieting bit on relevant targets.

@rustbot

This comment was marked as outdated.

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

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants