Skip to content

Make more float intrinsics generic - #162395

Open
N1ark wants to merge 8 commits into
rust-lang:mainfrom
N1ark:more-float-generics
Open

Make more float intrinsics generic#162395
N1ark wants to merge 8 commits into
rust-lang:mainfrom
N1ark:more-float-generics

Conversation

@N1ark

@N1ark N1ark commented Sep 7, 2026

Copy link
Copy Markdown
Member

View all comments

Another (big) step of #153834 + #160989

Makemore remaining float intrinsics generic! Each commit is self-contained.

In order:

The first commit just removes the f16/f128 fallbacks for fabs, as mentioned here. I didn't try making the backends use more fallbacks, to make the PR less risky since it's quite large. Happy to do changes there though (cc @folkertdev)

copysign, minimum_number_nsz, maximum_number_nsz, minimum, maximum will be handled separately as they require rustc_allow_const_fn_unstable: #162414

@rustbot

rustbot commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

miri is developed in its own repository. If the Miri part of this change can be broken out, consider making this change to rust-lang/miri instead. However, if Miri needs adjusting for rustc changes, just ignore this message.

cc @rust-lang/miri

rustc_codegen_gcc is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_gcc instead.

cc @antoyo, @GuillaumeGomez

compiler-builtins is developed in its own repository. If possible, consider making this change to rust-lang/compiler-builtins instead.

cc @tgross35

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

stdarch is developed in its own repository. If possible, consider making this change to rust-lang/stdarch instead.

cc @Amanieu, @folkertdev, @sayantn

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

⚠️ #[miri::intrinsic_fallback_is_spec] must only be used if the function actively checks for all UB cases,
and explores the possible non-determinism of the intrinsic.

cc @rust-lang/miri

Any special-casing of Miri in the standard library requires review.

cc @rust-lang/miri

rustc_codegen_cranelift is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_cranelift instead.

cc @bjorn3

⚠️ #[rustc_intrinsic_const_stable_indirect] controls whether intrinsics can be exposed to stable const
code; adding it needs t-lang approval.

cc @rust-lang/wg-const-eval

@rustbot rustbot added A-compiler-builtins Area: compiler-builtins (https://github.com/rust-lang/compiler-builtins) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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 7, 2026
@rustbot

rustbot commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

r? @JohnTitor

rustbot has assigned @JohnTitor.
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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: libs
  • libs expanded to 12 candidates
  • Random selection from Darksonn, JohnTitor, Mark-Simulacrum, clarfonthey

@rust-log-analyzer

This comment has been minimized.

Comment thread library/core/src/intrinsics/mod.rs
Comment thread library/core/src/intrinsics/mod.rs Outdated
Comment on lines 3099 to 3100
#[rustc_const_unstable(feature = "core_intrinsics", issue = "none")]
#[rustc_intrinsic_const_stable_indirect]

@RalfJung RalfJung Sep 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Noted this required adding #[rustc_const_unstable] to the whole intrinsic,

Why was this needed? I guess it is because the body now needs unstable const features, namely const traits? We should ensure const traits are stable enough before we do that.
(I now notice that you already did the same for fabs. We should have already checked this there then but forgot.)

@BoxyUwU @lcnr @oli-obk are const traits in a state where we can use them internally in const functions that can be called by public monomorphic stable const functions? I think this still allows us to change the syntax and logic for const traits pretty much arbitrarily as long as we keep some way of invoking trait functions in a const fn.

View changes since the review

@N1ark N1ark Sep 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yep, because it needs const_trait_impl, const_ops, const_cmp

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

these were moved to #162414

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.

cc @fee1-dead

This is not the only case of such usage. Equivalent behaviour is here to stay and syntactical changes don't cause bootstrap churn anymore, so to me personally it is fine to add more, especially for intrinsic bodies which can always be replaced by an actual impl again

Comment thread library/core/src/intrinsics/mod.rs Outdated
Comment on lines 3099 to 3100
#[rustc_const_unstable(feature = "core_intrinsics", issue = "none")]
#[rustc_intrinsic_const_stable_indirect]

@RalfJung RalfJung Sep 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This combination of attributes is a strange one.

#[rustc_const_unstable(feature = "core_intrinsics", issue = "none")]
#[rustc_intrinsic_const_stable_indirect]

I don't think I ever intended for this combination to be used. It looks quite unsafe, from a const-stability perspective. We should disallow it and require something like rustc_allow_const_fn_unstable instead.

View changes since the review

@RalfJung RalfJung Sep 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

fabs seems to be the only intrinsic using this. I am quite uncomfortable with adding more such intrinsics. If possible, we should remove this from fabs as well. Unfortunately I won't have time to work on fixing this hole in our const stability checks any time soon. (Well I can easily add the check that rejects this but probably cannot explore adding alternatives.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i admit this is me just trying to understand how to make things compile. very happy to do something else or exploring adding/fixing an attribute

  • without #[rustc_const_unstable(feature = "core_intrinsics", issue = "none")], we get "const function that might be (indirectly) exposed to stable cannot use #[feature(const_trait_impl)]"
  • without #[rustc_intrinsic_const_stable_indirect], we get "const function that might be (indirectly) exposed to stable cannot use #[feature(core_intrinsics)]"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed (folded into the relevant commits), there now is rustc_allow_const_fn_unstable; i guess this needs some approval

@N1ark
N1ark force-pushed the more-float-generics branch from d78924b to 06625b8 Compare September 7, 2026 08:16
@rustbot

rustbot commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

@rustbot rustbot added the T-clippy Relevant to the Clippy team. label Sep 7, 2026
@N1ark
N1ark force-pushed the more-float-generics branch from 06625b8 to da4599d Compare September 7, 2026 08:18
@rust-log-analyzer

This comment has been minimized.

@N1ark
N1ark force-pushed the more-float-generics branch 2 times, most recently from ca3f9a9 to b4275be Compare September 7, 2026 09:12
@rustbot

This comment was marked as resolved.

Comment thread library/core/src/intrinsics/mod.rs Outdated
@rust-log-analyzer

This comment has been minimized.

@N1ark
N1ark force-pushed the more-float-generics branch from b4275be to 49057d4 Compare September 7, 2026 09:53
@N1ark N1ark changed the title Make all float intrinsics generics Make more float intrinsics generic Sep 7, 2026
@RalfJung

RalfJung commented Sep 7, 2026

Copy link
Copy Markdown
Member

Please also update the PR description to reflect which intrinsics are actually being changed here now.

@rust-log-analyzer

This comment has been minimized.

Comment thread compiler/rustc_const_eval/src/interpret/intrinsics.rs
where
F: rustc_apfloat::Float + rustc_apfloat::FloatConvert<F> + Into<Scalar<M::Provenance>>,
{
let x: F = x.to_float()?;

@RalfJung RalfJung Sep 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By removing this here, now every caller needs to call to_float first... is that really better?

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i found it nicer, because it means outside of SIMD we only do the x: F = x.to_float()? thing in one place, avoiding additional wrappers or calls elsewhere. it does make the SIMD intrinsics slightly bigger but i think it's nicer for the more general case

@N1ark
N1ark force-pushed the more-float-generics branch from 49057d4 to 2bb83cf Compare September 7, 2026 10:10
@N1ark
N1ark force-pushed the more-float-generics branch from 2bb83cf to 33503c2 Compare September 7, 2026 10:12
Comment thread library/core/src/intrinsics/mod.rs Outdated
#[rustc_intrinsic]
#[rustc_nounwind]
pub fn powif128(a: f128, x: i32) -> f128;
pub fn powi<T: bounds::FloatPrimitive>(a: T, x: i32) -> T;

@beetrees beetrees Sep 7, 2026

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.

Could this intrinsic_dispatch_on_type! calling __powi[sdtk]f2 in the fallback body?

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes sure :) for f16, powi(x as f32, y) as f16 is correct right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

note that this requires declaring an extern block for these functions, since we dont have a compiler-builtins equivalent to libm. i'm happy adding a library/core/src/num/imp/builtins.rs for it, if you think it's not too much

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.

powi has no precision guarantees, so yes. You'll also need to put #[cfg_attr(any(target_arch = "powerpc", target_arch = "powerpc64"), link_name = "__powikf2")] on the __powitf2 extern fn as PowerPC uses slightly different names for f128 intrinsics.

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.

Adding library/core/src/num/imp/builtins.rs seems fine, e.g. complex number multiplication/division will also need to make direct calls to compiler-builtins.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in b6e3544

@@ -0,0 +1,14 @@
//! Bindings to functions provided by `compiler-builtins`.
//!
//! These are only ever provided by `compiler-builtins`, which is always linked in.

@beetrees beetrees Sep 7, 2026

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.

This isn't entirely true, compiler-builtins provides an implementation with weak linkage, meaning the implementation from e.g. libgcc_s or LLVM compiler-rt could also end up being used.

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

how about this?

//! Bindings to functions provided by `compiler-builtins`.
//!
//! These are weakly provided by `compiler-builtins`, which is always weakly linked in, or
//! possibly by e.g. libgcc_s or compiler-rt.

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

Labels

A-compiler-builtins Area: compiler-builtins (https://github.com/rust-lang/compiler-builtins) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-clippy Relevant to the Clippy team. 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