Skip to content

Implement the internal feature cfg_target_has_reliable_f16_f128 #140323

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Apr 26, 2025

Support for f16 and f128 is varied across targets, backends, and backend versions. Eventually we would like to reach a point where all backends support these approximately equally, but until then we have to work around some of these nuances of support being observable.

Introduce the cfg_target_has_reliable_f16_f128 internal feature, which provides the following new configuration gates:

  • cfg(target_has_reliable_f16)
  • cfg(target_has_reliable_f16_math)
  • cfg(target_has_reliable_f128)
  • cfg(target_has_reliable_f128_math)

reliable_f16 and reliable_f128 indicate that basic arithmetic for the type works correctly. The _math versions indicate that anything relying on libm works correctly, since sometimes this hits a separate class of codegen bugs.

These options match configuration set by the build script at 1. The logic for LLVM support is duplicated as-is from the same script. There are a few possible updates that will come as a follow up.

The config introduced here is not planned to ever become stable, it is only intended to replace the build scripts for std tests and compiler-builtins that don't have any way to configure based on the codegen backend.

MCP: rust-lang/compiler-team#866
Closes: rust-lang/compiler-team#866


The second commit makes use of this config to replace cfg_{f16,f128}{,_math} in library/. I omitted providing a cfg(bootstrap) configuration to keep things simpler since the next beta branch is in two weeks.

try-job: aarch64-gnu
try-job: i686-msvc-1
try-job: test-various
try-job: x86_64-gnu
try-job: x86_64-msvc-ext2

r? @ghost

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 26, 2025
@rust-log-analyzer

This comment has been minimized.

@tgross35 tgross35 added the rla-silenced Silences rust-log-analyzer postings to the PR it's added on. label Apr 26, 2025
@tgross35 tgross35 force-pushed the cfg-unstable-float branch 2 times, most recently from 98cf08a to e7fb9c5 Compare April 26, 2025 06:17
@tgross35
Copy link
Contributor Author

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 26, 2025
Implement the internal feature `cfg_target_has_reliable_f16_f128`

Support for `f16` and `f128` is varied across targets, backends, and backend versions. Eventually we would like to reach a point where all backends support these approximately equally, but until then we have to work around some of these nuances of support being observable.

Introduce the `target_has_reliable_f16_f128` configuration option, gated behind `cfg_target_has_reliable_f16_f128`, as an indicator for whether or not the backend supports these types to a point that they can and should be tested. The configuration mostly follows the logic used by `target_feature`, and similarly takes a value:

    #[cfg(target_has_reliable_f16_f128 = "f16")]

Accepted values are:

* `f16`
* `f16-math`
* `f128`
* `f128-math`

`f16` and `f128` indicate that basic arithmetic for the type works correctly. The `-math` versions indicate that anything relying on `libm` works correctly, since sometimes this hits a separate class of codegen bugs.

These options match configuration set by the build script at [1]. The logic for LLVM support is duplicated as-is from the same script. There are a few possible updates that will come as a follow up.

The config introduced here is not intended to ever become stable, it is only intended to replace the build scripts for `std` tests and in `compiler-builtins` that don't have any way to set configuration based on the codegen backend.

MCP: rust-lang/compiler-team#866
Closes: rust-lang/compiler-team#866

[1]: https://github.com/rust-lang/rust/blob/555e1d0386f024a8359645c3217f4b3eae9be042/library/std/build.rs#L84-L186

---

The second commit makes use of this config to replace `cfg_{f16,f128}{,_math}` in `library/`.

try-job: aarch64-gnu
try-job: i686-msvc-1
try-job: test-various
try-job: x86_64-gnu
try-job: x86_64-msvc-ext2
@bors
Copy link
Collaborator

bors commented Apr 26, 2025

⌛ Trying commit e7fb9c5 with merge 4055b73...

@tgross35 tgross35 marked this pull request as ready for review April 26, 2025 06:42
@rustbot
Copy link
Collaborator

rustbot commented Apr 26, 2025

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in cfg and check-cfg configuration

cc @Urgau

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@tgross35
Copy link
Contributor Author

r? compiler

Cc @bjorn3 @antoyo since this touches the backend interfaces.

@tgross35 tgross35 added the F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` label Apr 26, 2025
@tgross35 tgross35 force-pushed the cfg-unstable-float branch from e7fb9c5 to 4d7bb41 Compare April 26, 2025 07:16
@rustbot
Copy link
Collaborator

rustbot commented Apr 26, 2025

Some changes occurred in src/doc/rustc/src/check-cfg.md

cc @Urgau

@tgross35
Copy link
Contributor Author

Hm... I think I'm actually leaning toward four separate key-only cfg options here rather than a single key with multiple values. I don't think the current version works as-well, it's a bit bulky and redundant with f16_f128 in the name.

Will change it over tomorrow unless anyone has a stronger preference.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 26, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 26, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@tgross35 tgross35 force-pushed the cfg-unstable-float branch 2 times, most recently from b1025c9 to 5abd112 Compare April 26, 2025 20:14
@tgross35
Copy link
Contributor Author

Changed so this is split across four different cfg options.

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 26, 2025
@tgross35 tgross35 force-pushed the cfg-unstable-float branch 2 times, most recently from ac61743 to 304739d Compare April 27, 2025 17:41
@tgross35
Copy link
Contributor Author

tgross35 commented Apr 27, 2025

Since you have already started taking a look,

r? @Urgau

@rustbot

This comment was marked as outdated.

@rustbot rustbot assigned Urgau and unassigned jieyouxu Apr 27, 2025
@tgross35 tgross35 requested a review from Urgau April 27, 2025 18:11
Comment on lines +246 to +253
/// Option for `cfg(target_has_reliable_f16)`, true if `f16` basic arithmetic works.
pub has_reliable_f16: bool,
/// Option for `cfg(target_has_reliable_f16_math)`, true if `f16` math calls work.
pub has_reliable_f16_math: bool,
/// Option for `cfg(target_has_reliable_f128)`, true if `f128` basic arithmetic works.
pub has_reliable_f128: bool,
/// Option for `cfg(target_has_reliable_f128_math)`, true if `f128` math calls work.
pub has_reliable_f128_math: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Reliability of f16/f128 is not related to target features.

I wonder if we could come up with a better name of the query (and this struct). Maybe simply TargetConfig or TargetCodegenCfg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, this doesn't really directly affect codegen so I went with TargetConfig.

let target_abi = sess.target.options.abi.as_ref();
let target_pointer_width = sess.target.pointer_width;

cfg.has_reliable_f16 = match (target_arch, target_os) {
Copy link
Member

Choose a reason for hiding this comment

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

Miri is no longer tested here, were the intrinsics ever implemented? or do we have a different mechanism for Miri?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe they are still unimplemented but I haven't checked in a while (I assume this would be updated if they were added). But that should probably be done separately even if it works now, so I have just been replacing the existing cfg(reliable_f16_math) with #[cfg(not(miri))] and #[cfg(target_has_reliable_f16)].

I did forget to include cfg(not(miri)) on the library doctests though, fixed that now.

Comment on lines +25 to +26
/// # #![cfg_attr(not(bootstrap), feature(cfg_target_has_reliable_f16_f128))]
/// # #![cfg_attr(not(bootstrap), expect(internal_features))]
Copy link
Member

Choose a reason for hiding this comment

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

To bad that rustdoc #![doc(test(attr(...)))] attribute only works at the crate-root level (and not module level), otherwise we could have saved us a lot of redundancy by just having in each module:

#![doc(test(attr(feature(cfg_target_has_reliable_f16_f128))))]
#![doc(test(attr(expect(internal_features))))]

Copy link
Contributor Author

@tgross35 tgross35 Apr 27, 2025

Choose a reason for hiding this comment

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

Oh that would be so nice. Agreed, tests are too noisy with config, but at least they shouldn't need to be updated very frequently. (and bootstrap will go away soon)

Support for `f16` and `f128` is varied across targets, backends, and
backend versions. Eventually we would like to reach a point where all
backends support these approximately equally, but until then we have to
work around some of these nuances of support being observable.

Introduce the `cfg_target_has_reliable_f16_f128` internal feature, which
provides the following new configuration gates:

* `cfg(target_has_reliable_f16)`
* `cfg(target_has_reliable_f16_math)`
* `cfg(target_has_reliable_f128)`
* `cfg(target_has_reliable_f128_math)`

`reliable_f16` and `reliable_f128` indicate that basic arithmetic for
the type works correctly. The `_math` versions indicate that anything
relying on `libm` works correctly, since sometimes this hits a separate
class of codegen bugs.

These options match configuration set by the build script at [1]. The
logic for LLVM support is duplicated as-is from the same script. There
are a few possible updates that will come as a follow up.

The config introduced here is not planned to ever become stable, it is
only intended to replace the build scripts for `std` tests and
`compiler-builtins` that don't have any way to configure based on the
codegen backend.

MCP: rust-lang/compiler-team#866
Closes: rust-lang/compiler-team#866

[1]: https://github.com/rust-lang/rust/blob/555e1d0386f024a8359645c3217f4b3eae9be042/library/std/build.rs#L84-L186
@tgross35 tgross35 force-pushed the cfg-unstable-float branch from 304739d to f32222b Compare April 27, 2025 19:59
New compiler configuration has been introduced that is designed to
replace the build script configuration `reliable_f16`, `reliable_f128`,
`reliable_f16_math`, and `reliable_f128_math`. Do this replacement here,
which allows us to clean up `std`'s build script.

All tests are gated by `#[cfg(bootstrap)]` rather than doing a more
complicated `cfg(bootstrap)` / `cfg(not(bootstrap))` split since the
next beta split is within two weeks.
@tgross35 tgross35 force-pushed the cfg-unstable-float branch from f32222b to dfa972e Compare April 27, 2025 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` rla-silenced Silences rust-log-analyzer postings to the PR it's added on. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add #[cfg(target_has_unstable_float = "...")]
6 participants