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

Make some float methods unstable const fn #130568

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

Conversation

eduardosm
Copy link
Contributor

@eduardosm eduardosm commented Sep 19, 2024

Some float methods are now const fn under the const_float_methods feature gate (except f16 and f128 versions, which I placed under f16 and f128 feature gates respectively).

I also made some unstable methods const fn, keeping their constness under their respective feature gate.

In order to support min / max, the implementation of some intrinsics had to be moved from Miri to rustc_const_eval (cc @RalfJung).

Tracking issue: #130843

impl <float> {
    // #[feature(const_float_methods)]
    pub const fn recip(self) -> Self;
    pub const fn to_degrees(self) -> Self;
    pub const fn to_radians(self) -> Self;
    pub const fn max(self, other: Self) -> Self;
    pub const fn min(self, other: Self) -> Self;
    pub const fn clamp(self, min: Self, max: Self) -> Self;
    pub const fn abs(self) -> Self;
    pub const fn signum(self) -> Self;
    pub const fn copysign(self, sign: Self) -> Self;

    // #[feature(float_minimum_maximum)]
    pub const fn maximum(self, other: Self) -> Self;
    pub const fn minimum(self, other: Self) -> Self;

    // Only f16/f128 (f32/f64 already const)
    pub const fn is_sign_positive(self) -> bool;
    pub const fn is_sign_negative(self) -> bool;
    pub const fn next_up(self) -> Self;
    pub const fn next_down(self) -> Self;
}

r? libs-api

@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 Sep 19, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 19, 2024

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

@jieyouxu jieyouxu added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 19, 2024
@RalfJung
Copy link
Member

r=me on the interpreter and Miri parts.

@eduardosm
Copy link
Contributor Author

I made clamp const too, using an approach similar to #130611 to handle the assert formatting.

@rust-log-analyzer

This comment has been minimized.

@tgross35
Copy link
Contributor

If this is accepted, a tracking issue should be created (for now I set issue = "none").

You can go ahead and do this in advance, I don't think there is any reason these would be rejected

Comment on lines +474 to +475
#[rustc_const_unstable(feature = "f128", issue = "116909")]
pub const fn is_sign_positive(self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for including f16 and f128 :) would you mind adding a commented out gate on methods from this feature? E.g.

    // #[rustc_const_unstable(feature = "const_float_methods", issue = "...")]
    #[rustc_const_unstable(feature = "f128", issue = "116909")]
    pub const fn is_sign_positive(self) -> bool {

We just do that to keep things somewhat linked if API falls under >1 gate.

@rust-log-analyzer

This comment has been minimized.

@eduardosm
Copy link
Contributor Author

Created tracking issue: #130843

For f16/f32 methods, I placed commented gates under the same feature as f32/f64 (e.g., is_sign_positive under const_float_classify, next_up under float_next_up_down).

@bors
Copy link
Contributor

bors commented Oct 4, 2024

☔ The latest upstream changes (presumably #130157) made this pull request unmergeable. Please resolve the merge conflicts.

@eduardosm
Copy link
Contributor Author

I believe is_sign_positive / is_sign_negative do not need the second unstable gate since const_float_classify is now stable.

@@ -249,28 +249,18 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
this.write_scalar(res, dest)?;
}

"minnumf32" | "maxnumf32" | "copysignf32" => {
"copysignf32" => {
Copy link
Member

Choose a reason for hiding this comment

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

Btw, is there a reason you are not including copy_sign in this?

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 could move it to rustc_const_eval. But, do we want to make copysign and abs const? Ideally, we'd want to, but since they are implemented in libstd instead of libcore, I'm not sure what's the deal with those.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see why we wouldn't make them const.

I also don't know why they are in std, maybe the intrinsic is sometimes compiled to a libcall or so... let's just leave them where they are. 🤷

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'll make them const. But I find it quite weird that we have operations like min/max or % in libcore, while we are afraid to have operations like abs or copysign that can be trivially implemented with bitwise operations.

Copy link
Member

@RalfJung RalfJung Oct 5, 2024

Choose a reason for hiding this comment

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

I fully agree. Maybe file an issue for that?

EDIT: Ah, an issue already exists: #50145

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 made abs, copysign and signum (which uses copysign) const.

}
}

enum FloatBinOp {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enum FloatBinOp {
enum FloatBinIntrinsic {

We use BinOp for the MIR binops, IMO we shouldn't mix up that terminology.

Also please move this above the functions that use it.

@@ -0,0 +1,46 @@
//@ run-pass
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//@ run-pass
//@ run-pass
//! Tests the float intrinsics: min, max, abs, copysign

@RalfJung
Copy link
Member

RalfJung commented Oct 5, 2024

LGTM from the interpreter side.

r? libs-api

@eduardosm
Copy link
Contributor Author

Rebased to pick #131256 and remove #![feature(f16_const)] and #![feature(f128_const)].

@bors
Copy link
Contributor

bors commented Oct 12, 2024

☔ The latest upstream changes (presumably #131581) made this pull request unmergeable. Please resolve the merge conflicts.

Some float methods are now `const fn` under the `const_float_methods` feature gate (except `f16` and `f128` versions, which I placed under `f16` and `f128` feature gates respectively).

In order to support `min` / `max`, the implementation of some intrinsics had to be moved from Miri to rustc_const_eval.
@tgross35
Copy link
Contributor

tgross35 commented Oct 12, 2024

If libs-api can just okay unstably adding const to the signatures, I'm happy to review the rest.

@rustbot label +I-libs-api-nominated

@rustbot rustbot added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-libs-api-nominated Nominated for discussion during a libs-api team meeting. 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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants