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

Add constants for f16 and f128 #123850

Merged
merged 5 commits into from
May 6, 2024
Merged

Conversation

tspiteri
Copy link
Contributor

  • Commit 1 adds associated constants for f16, excluding NaN and infinities as these are implemented using arithmetic for f32 and f64.
  • Commit 2 adds associated constants for f128, excluding NaN and infinities.
  • Commit 3 adds constants in std::f16::consts.
  • Commit 4 adds constants in std::f128::consts.

@rustbot
Copy link
Collaborator

rustbot commented Apr 12, 2024

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Apr 12, 2024
@tspiteri tspiteri changed the title Add constants for f16 and f32 Add constants for f16 and f128 Apr 12, 2024
@fmease
Copy link
Member

fmease commented Apr 12, 2024

cc @tgross35

@rust-log-analyzer

This comment has been minimized.

@tspiteri tspiteri force-pushed the f16_f128_consts branch 3 times, most recently from 9810c6e to e972a7f Compare April 12, 2024 13:30
@rust-log-analyzer

This comment has been minimized.

@tspiteri
Copy link
Contributor Author

Note that for constants gated under more_float_constants for f32 and f64, I used #103883 rather than #116909 as a tracking issue.

@tspiteri
Copy link
Contributor Author

The test failure seems to be a panic in beta clippy.

  thread 'rustc' panicked at src/tools/clippy/clippy_lints/src/float_literal.rs:145:26:
  not implemented: f16_f128

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Hey, thank you for the help here!

I was holding off on these until we had the ability to actually run code that involves these types, which unfortunately we won't until I get compiler_builtins updated (well, code will run on x86 but nothing else so we can't actually merge anything). I was trying to add as little as possible until we get tests, but don't have any big problem with adding these.

One change needed - the literals we have aren't high enough precision for f128. I have a Julia script to generate longer ones using arbitrary precision math, https://github.com/tgross35/apfloat-consts. You can just copy the consts from output.rs.

@tgross35
Copy link
Contributor

The test failure seems to be a panic in beta clippy.

  thread 'rustc' panicked at src/tools/clippy/clippy_lints/src/float_literal.rs:145:26:
  not implemented: f16_f128

This is the other reason I haven't added too much more, clippy and CTFE both have some unimplemented! scattered around that can't be fixed until math is working (compiler_builtins...). You might be able to #[allow(clippy::all)] on this module.

@rustbot label +F-f16_and_f128

@rustbot rustbot added the F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` label Apr 12, 2024
@tspiteri
Copy link
Contributor Author

Oops, I used 36 significant digits for f128 constants. I stumbled because 36 decimal digits is enough for numbers which are representable (such as EPSILON, MIN_POSITIVE, MAX, etc.), but not for numbers that are not, where you may need many more digits especially if the exact decimal number is close to mid-way between two consecutive floating-point numbers.

I changed f128 constants like PI to use 60 decimal significant digits, which is enough for all these constants. (I generated them with MPFR through the Rug crate, with rounding to the nearest just like the constants in the standard library for f32 and f64.) For f16 I just used the same literals that are used for f32 and f64.

@tspiteri
Copy link
Contributor Author

For the clippy warning, I think clippy is already fine in nightly, so instead of adding extra clippy allows, I think I might just wait for the next beta to be cut.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

tspiteri added 4 commits May 2, 2024 15:13
NaN and infinity are not included as they require arithmetic.
NaN and infinity are not included as they require arithmetic.
@tspiteri tspiteri force-pushed the f16_f128_consts branch from 37bec59 to d8e6b81 Compare May 2, 2024 13:13
@tspiteri
Copy link
Contributor Author

tspiteri commented May 2, 2024

For the clippy warning, I think clippy is already fine in nightly, so instead of adding extra clippy allows, I think I might just wait for the next beta to be cut.

I rebased (to retrigger the tests) now that the clippy tests in mingw-check are using 2024-04-29 instead of 2024-03-19, and the tests now pass.

@tgross35
Copy link
Contributor

tgross35 commented May 2, 2024

I changed f128 constants like PI to use 60 decimal significant digits, which is enough for all these constants. (I generated them with MPFR through the Rug crate, with rounding to the nearest just like the constants in the standard library for f32 and f64.) For f16 I just used the same literals that are used for f32 and f64.

Could you post the code for this so we can reproduce in the future?

@tspiteri
Copy link
Contributor Author

tspiteri commented May 2, 2024

This is my code below. Should I include the code in a comment in f128.rs?

use rug::float::Constant;
use rug::{Assign, Float};

fn decimal_string(val: &Float, num_digits: usize) -> String {
    let (sign, mut digits, exp) = val.to_sign_string_exp(10, Some(num_digits));
    if let Some(exp) = exp {
        if exp <= 0 {
            let one_minus_exp = usize::try_from(1 - exp).unwrap();
            digits.insert_str(0, &"0".repeat(one_minus_exp));
            digits.insert(1, '.');
        } else {
            let exp = usize::try_from(exp).unwrap();
            if exp >= num_digits {
                digits.push_str(&"0".repeat(exp - num_digits));
            } else {
                digits.insert(exp, '.');
            }
        }
    }
    if sign {
        digits.insert(0, '-');
    }
    digits
}

fn float<T>(t: T) -> Float
where
    Float: Assign<T>,
{
    Float::with_val(1000, t)
}

fn print(name: &str, f: Float) {
    let s = decimal_string(&f, 60);
    assert_eq!(
        Float::with_val(113, Float::parse(&s).unwrap()),
        Float::with_val(113, &f)
    );
    println!("pub const {name}: f128 = {s}_f128;");
}

fn main() {
    print("PI", float(Constant::Pi));
    print("TAU", float(Constant::Pi) * 2);
    print("PHI", float(1.25).sqrt() + 0.5);
    print("EGAMMA", float(Constant::Euler));
    print("FRAC_PI_2", float(Constant::Pi) / 2);
    print("FRAC_PI_3", float(Constant::Pi) / 3);
    print("FRAC_PI_4", float(Constant::Pi) / 4);
    print("FRAC_PI_6", float(Constant::Pi) / 6);
    print("FRAC_PI_8", float(Constant::Pi) / 8);
    print("FRAC_1_PI", float(Constant::Pi).recip());
    print("FRAC_1_SQRT_PI", float(Constant::Pi).sqrt().recip());
    print("FRAC_2_PI", 2 / float(Constant::Pi));
    print("FRAC_2_SQRT_PI", 2 / float(Constant::Pi).sqrt());
    print("SQRT_2", float(2).sqrt());
    print("FRAC_1_SQRT_2", float(0.5).sqrt());
    print("SQRT_3", float(3).sqrt());
    print("FRAC_1_SQRT_3", float(3).sqrt().recip());
    print("E", float(1).exp());
    print("LOG2_10", float(10).log2());
    print("LOG2_E", float(1).exp().log2());
    print("LOG10_2", float(2).log10());
    print("LOG10_E", float(1).exp().log10());
    print("LN_2", float(2).ln());
    print("LN_10", float(10).ln());
}

@tgross35
Copy link
Contributor

tgross35 commented May 2, 2024

No this is fine, just wanted a reference in case anybody wants to replicate or add more in the future 👍

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Okay, these all look good to me. I double checked the associated values against http://weitz.de/ieee/ and the math constants against what Julia says https://github.com/tgross35/apfloat-consts/blob/49cb0d131ea2d881205852c31ba87236fdba9749/output.rs.

Honestly it would be better to just generate bit patterns for the MIN/MAX/EPSILON/etc values rather than having a tough to read literal, but that's not something to do now.

I can't r+ so somebody else has to. Thanks for the patch!

Comment on lines +164 to +186
/// Smallest finite `f128` value.
///
/// Equal to &minus;[`MAX`].
///
/// [`MAX`]: f128::MAX
#[unstable(feature = "f128", issue = "116909")]
pub const MIN: f128 = -1.18973149535723176508575932662800701e+4932_f128;
/// Smallest positive normal `f128` value.
///
/// Equal to 2<sup>[`MIN_EXP`]&nbsp;&minus;&nbsp;1</sup>.
///
/// [`MIN_EXP`]: f128::MIN_EXP
#[unstable(feature = "f128", issue = "116909")]
pub const MIN_POSITIVE: f128 = 3.36210314311209350626267781732175260e-4932_f128;
/// Largest finite `f128` value.
///
/// Equal to
/// (1&nbsp;&minus;&nbsp;2<sup>&minus;[`MANTISSA_DIGITS`]</sup>)&nbsp;2<sup>[`MAX_EXP`]</sup>.
///
/// [`MANTISSA_DIGITS`]: f128::MANTISSA_DIGITS
/// [`MAX_EXP`]: f128::MAX_EXP
#[unstable(feature = "f128", issue = "116909")]
pub const MAX: f128 = 1.18973149535723176508575932662800701e+4932_f128;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor style nit, would be good to consistently keep a line between the constant and docs (I know this was just copied from the existing floats)

@Amanieu
Copy link
Member

Amanieu commented May 6, 2024

@bors r+

@bors
Copy link
Contributor

bors commented May 6, 2024

📌 Commit d8e6b81 has been approved by Amanieu

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 May 6, 2024
@bors
Copy link
Contributor

bors commented May 6, 2024

⌛ Testing commit d8e6b81 with merge fc47cf3...

@bors
Copy link
Contributor

bors commented May 6, 2024

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing fc47cf3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 6, 2024
@bors bors merged commit fc47cf3 into rust-lang:master May 6, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 6, 2024
@tspiteri tspiteri deleted the f16_f128_consts branch May 6, 2024 21:03
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fc47cf3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

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.

mean range count
Regressions ❌
(primary)
1.8% [1.8%, 1.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.8% [1.8%, 1.8%] 1

Cycles

Results

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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
6.1% [6.0%, 6.3%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 676.836s -> 676.942s (0.02%)
Artifact size: 315.81 MiB -> 315.88 MiB (0.02%)

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)]` merged-by-bors This PR was explicitly merged by bors. 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.

8 participants