Skip to content

feat: add const support for float rounding methods #141521

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 18 commits into
base: master
Choose a base branch
from

Conversation

ruancomelli
Copy link
Contributor

Add const support for float rounding methods

This PR makes the following float rounding methods const:

  • f64::{floor, ceil, trunc, round, round_ties_even}
  • and the corresponding methods for f16, f32 and f128

Procedure

I followed c09ed3e as closely as I could in making float methods const, and also received great guidance from https://internals.rust-lang.org/t/const-rounding-methods-in-float-types/22957/3?u=ruancomelli.

Question regarding the "unstability" attributes

I did my best to apply the appropriate const stability attributes, but I would greatly appreciate feedback or corrections:

  • #[rustc_allow_const_fn_unstable(core_intrinsics)] in library/core/src/intrinsics/mod.rs
  • #[rustc_const_unstable(feature = "f16", issue = "116909")] in library/core/src/num/f16.rs
  • #[rustc_const_unstable(feature = "f128", issue = "116909")] in library/core/src/num/f128.rs
  • #[rustc_const_unstable(feature = "const_float_methods", issue = "130843")] in library/core/src/num/32.rs and library/core/src/num/f64.rs

Please let me know if any of these are incorrect or should be revised.

Alternative implementation

I implemented one intrinsic method (float_<...>_intrinsic) for each one of the rounding methods considered in this PR (floor, ceil, trunc, round and round_ties_even). This is similar to how other intrinsics are implemented in compiler/rustc_const_eval/src/interpret/intrinsics.rs.

However, I could have implemented a single one taking a rustc_apfloat::Round parameter:

    fn float_round_intrinsic<F>(
        &mut self,
        args: &[OpTy<'tcx, M::Provenance>],
        dest: &MPlaceTy<'tcx, M::Provenance>,
        mode: rustc_apfloat::Round
    ) -> InterpResult<'tcx, ()>
    where
        F: rustc_apfloat::Float + rustc_apfloat::FloatConvert<F> + Into<Scalar<M::Provenance>>,
    {
        let x: F = self.read_scalar(&args[0])?.to_float()?;
        let res = x.round_to_integral(mode).value;
        self.write_scalar(res, dest)?;
        interp_ok(())
    }

This is, of course, shorter than the current implementation. I'd be happy to change to this version if preferred.

Note

This is my first code contribution to the Rust project, so please let me know if I missed anything - I'd be more than happy to revise and learn more. Thank you for taking the time to review it!

@rustbot
Copy link
Collaborator

rustbot commented May 24, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 24, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 24, 2025

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

@RalfJung
Copy link
Member

However, I could have implemented a single one taking a rustc_apfloat::Round parameter:

Yes, that's a lot better I think. :)

@RalfJung
Copy link
Member

Note that your branch has conflicts with the master branch. Not sure how that happened given that the PR is fresh, but it means you'll have to rebase and resolve those conflicts.

@ruancomelli ruancomelli force-pushed the const-float-rounding branch from bed4525 to 372e4cd Compare May 24, 2025 21:49
@rustbot

This comment has been minimized.

@ruancomelli
Copy link
Contributor Author

Strange, CI is failing with intrinsic is not supported at compile-time for each one of the new methods.
I'm looking into that.

@ruancomelli
Copy link
Contributor Author

ruancomelli commented May 25, 2025

As far as I can see, the main difference between the newly const methods (e.g. floor) and the ones that already work (e.g. abs) is the attributes. So my current hypothesis is that I got them wrong.

Copy link
Contributor

@CAD97 CAD97 left a comment

Choose a reason for hiding this comment

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

AIUI, the reason you're hitting "intrinsic not supported at compile time" is that std currently gets built with the bootstrap compiler, which is then used when building the new compiler. So you need to have a #[cfg(bootstrap)] version of the function/intrinsic which isn't const and a #[cfg(not(bootstrap))] which is.

IIUC, we're in the process of flipping this so that the compiler gets built with the bootstrap std, and std always gets built with the new compiler. Once that switch happens, this won't be necessary anymore.

@RalfJung
Copy link
Member

AIUI, the reason you're hitting "intrinsic not supported at compile time" is that std currently gets built with the bootstrap compiler, which is then used when building the new compiler. So you need to have a #[cfg(bootstrap)] version of the function/intrinsic which isn't const and a #[cfg(not(bootstrap))] which is.

The problem occurs when building the tests. So you don't need to do any cfg in libcore itself, but the new tests in library/coretests need a #[cfg(not(bootstrap))].

@RalfJung
Copy link
Member

RalfJung commented May 25, 2025 via email

@rust-log-analyzer

This comment has been minimized.

@ruancomelli
Copy link
Contributor Author

Hey @RalfJung and @CAD97, I've addressed or replied to all of your review comments.

I've also created a tracking issue for this feature here: #141555.

I have two main open questions now.

First one is: should the rounding methods on f16 and f128 be gated behind #[rustc_const_unstable(feature = {"f16", "f128"}, issue = "116909")] (from #116909) or #[rustc_const_unstable(feature = "const_float_round_methods", issue = "141555")]? I think it should be the former one, but I'm not sure.

The second question: I added #![feature(const_float_round_methods)] to library/std/src/lib.rs, which was suggested by the compiler itself. Without it, ./x check results in the following errors:

error: `core::f32::math::floor` is not yet stable as a const fn
  --> library/std/src/f32.rs:50:9
   |
50 |         core::f32::math::floor(self)
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
help: add `#![feature(const_float_round_methods)]` to the crate attributes to enable
  --> library/std/src/lib.rs:433:1
   |
433+ #![feature(const_float_round_methods)]
   |

... and similar for the other rounding methods

error: could not compile `std` (lib) due to 12 previous errors

This is similar to what was done in the reference PR, even though they changed library/core/src/lib.rs (notice core and not std).
However, I'm not sure if this is the correct way to proceed. I read the dev guide book, but that question remained unanswered.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member

RalfJung commented May 25, 2025

First one is: should the rounding methods on f16 and f128 be gated behind #[rustc_const_unstable(feature = {"f16", "f128"}, issue = "116909")] (from #116909) or #[rustc_const_unstable(feature = "const_float_round_methods", issue = "141555")]? I think it should be the former one, but I'm not sure.

Good question! The typical thing we do is to gate it behind the f16/f128 feature, but then also have a second commented out more specific feature gate:

#[rustc_const_unstable(feature = "f16", issue = "116909")]
// #[rustc_const_unstable(feature = "const_float_round_methods", issue = "141555")]
pub const fn trunc(self) -> self { ... }

The second question: I added #![feature(const_float_round_methods)] to library/std/src/lib.rs, which was suggested by the compiler itself.

Yes, that's fine.
Thanks for asking for confirmation. :)

@RalfJung
Copy link
Member

@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 May 25, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 25, 2025

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

@@ -924,6 +924,96 @@ macro_rules! test_float {
assert!($inf.div_euclid($nan).is_nan());
assert!($nan.div_euclid($inf).is_nan());
}
#[test]
#[cfg(not(bootstrap))]
fn floor() {
Copy link
Member

@RalfJung RalfJung May 26, 2025

Choose a reason for hiding this comment

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

FWIW we already have floor tests in library/coretests/tests/floats/f*.rs (and same for the other rounding methods). We probably shouldn't duplicate tests, but also those tests can't easily access the "test the same thing in const and regular fn" infrastructure that we have here.

@tgross35 how would you like these tests to be organized? Is it okay to move them from tests/float/f*.rs to here?

Copy link
Member

Choose a reason for hiding this comment

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

It's honestly kind of strange to have float tests in num at all given that the core::num module hardly has any float stuff... but it is described as being for "numeric" types, not just integers. Also this is the wrong PR to move everything around.^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably shouldn't duplicate tests, but also those tests can't easily access the "test the same thing in const and regular fn" infrastructure that we have here.

But the "test the same thing in const and regular" infrastructure is only used for the float tests, so we could move this infra into library/coretests/tests/floats, could we not?

Also this is the wrong PR to move everything around.

Phew! 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

@tgross35 how would you like these tests to be organized? Is it okay to move them from tests/float/f*.rs to here?

I think that's probably fine in general if needed, but things are bound to get a bit tricky. The f18/f128 functions here need cfg(target_has_reliable_f16_math) gating as well as cfg(miri) since that isn't implemented yet. There are also a few cases where the different sizes need different results (maybe not relevant here?).

Copy link
Contributor

@tgross35 tgross35 May 28, 2025

Choose a reason for hiding this comment

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

As a thought, what if coretests/tests/floats/mod.rs (or coretests/tests/num.rs, whatever) defined macros that expand to test cases, rather than entire tests? So each float type defines its own test functions that can include type-specific tests, but some of the copy+paste and other redundancy is gone.

E.g.:

// coretests/tests/floats/f32.rs

#[test]
fn test_fract() {
    shared_fract_tests!(f32); // tests used by all sizes
    assert_eq!(f32::math::fract(1.3f32), 0.29999995f32); // type-specific tests
}

Separately, it seems like it would be useful to have a #[test_const] proc macro that duplicates the test and wraps it in a const block with redefined assert* macros. Since there are a growing number of cases where we want to be able to verify behavior both at CTFE and at runtime.

Copy link
Member

@RalfJung RalfJung May 28, 2025

Choose a reason for hiding this comment

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

But the "test the same thing in const and regular" infrastructure is only used for the float tests, so we could move this infra into library/coretests/tests/floats, could we not?

Yeah long-term that's the right plan IMO.

As a thought, what if coretests/tests/floats/mod.rs (or coretests/tests/num.rs, whatever) defined macros that expand to test cases, rather than entire tests? So each float type defines its own test functions that can include type-specific tests, but some of the copy+paste and other redundancy is gone.

That seems like it'd make it harder to ensure we truly do have all the same tests for all float types though. I don't think we want to have f16.rs and f32.rs and so on on the test side.

Separately, it seems like it would be useful to have a #[test_const] proc macro that duplicates the test and wraps it in a const block with redefined assert* macros. Since there are a growing number of cases where we want to be able to verify behavior both at CTFE and at runtime.

I think something reasonably close can even be done as a regular macro.

@ruancomelli For this PR, maybe leave a FIXME comment both at the new and the old tests that this needs deduplicating? It'd also be worth opening an issue to track this and reference it from the FIXME.

Copy link
Member

Choose a reason for hiding this comment

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

Or, if the new tests do cover everything the existing test_floor have, remove the old ones, and file an issue for figuring out the organization here.

@RalfJung
Copy link
Member

r? @RalfJung

@rustbot rustbot assigned RalfJung and unassigned ibraheemdev May 26, 2025
@ruancomelli ruancomelli force-pushed the const-float-rounding branch from 4512002 to 74e3628 Compare May 27, 2025 23:23
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. 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.

7 participants