intrinsics: Add a fallback for non-const libm float functions#150946
Open
tgross35 wants to merge 3 commits into
Open
intrinsics: Add a fallback for non-const libm float functions#150946tgross35 wants to merge 3 commits into
tgross35 wants to merge 3 commits into
Conversation
Collaborator
rustbot
added
S-waiting-on-review
tgross35
commented
Jan 10, 2026
Comment on lines
1410
to
1413
| #[rustc_intrinsic_const_stable_indirect] | ||
| #[rustc_intrinsic] | ||
| #[rustc_nounwind] | ||
| pub const fn fmaf128(a: f128, b: f128, c: f128) -> f128; |
Contributor
Author
There was a problem hiding this comment.
Is there a way to provide a non-const fallback for const intrinsics? In cases where CTFE definitely has to hook it rather than using the fallback, like here.
Not the worst thing if not.
Member
There was a problem hiding this comment.
I don't think there is -- please file an issue.
This comment has been minimized.
This comment has been minimized.
tgross35
force-pushed
the
float-intrinsic-fallback
branch
from
d533ef5 to
90b9d6a
Compare
This comment has been minimized.
This comment has been minimized.
Contributor
Author
Huh - so |
Member
|
At a glance, your reasoning is completely backwards. The fallback body for |
Contributor
Author
|
It shouldn't be; the |
Member
|
The changes LGTM. However I can't actually check whether what you say about what is available where is correct -- you would be the person I ask about things like that. ;) |
Contributor
Author
|
https://github.com/rust-lang/compiler-builtins/blob/65624df7f55db9b7b494fbe3aa9dcea0a743eea4/compiler-builtins/src/math/mod.rs is what controls what we sometimes/always provide, so the module root symbols and |
Member
Yes, that's the problem. The body for the function is not available in the crate compiler-builtins when compiling compiler-builtins. Remember the the rule is to ban linkage against other crates and the point of these extern declarations is to use linkage. |
This comment has been minimized.
This comment has been minimized.
Contributor
Author
Forgot to follow up here. In this case it there isn't anything technically incorrect right, and instead it's a limitation of the knowledge the compiler has to emit that error? Because c-b itself provides the It probably goes without saying but libm isn't calling the |
Member
|
Oh I see, thanks for clarifying. I think this is actually a limitation of the check logic itself. The problem is that you have an item defined in an upstream crate, which is not So the missing check is whether the |
Member
|
I threw this together to convince myself that this can work, and it seems to work. diff --git a/compiler/rustc_codegen_ssa/src/base.rs b/compiler/rustc_codegen_ssa/src/base.rs
index c8aa7c04585..3fc0411d2be 100644
--- a/compiler/rustc_codegen_ssa/src/base.rs
+++ b/compiler/rustc_codegen_ssa/src/base.rs
@@ -867,6 +867,10 @@ pub fn codegen_crate<B: ExtraBackendMethods>(
/// unlinkable calls.
///
/// Note that calls to LLVM intrinsics are uniquely okay because they won't make it to the linker.
+/// Note also that calls to foreign items that are actually exported by the local crate are also
+/// okay. This situation arises because compiler-builtins calls functions in core that are #[inline]
+/// wrappers for extern "C" declarations in core, which resolve to a symbol exported by
+/// compiler-builtins.
pub fn is_call_from_compiler_builtins_to_upstream_monomorphization<'tcx>(
tcx: TyCtxt<'tcx>,
instance: Instance<'tcx>,
@@ -879,11 +883,19 @@ fn is_llvm_intrinsic(tcx: TyCtxt<'_>, def_id: DefId) -> bool {
}
}
+ fn is_extern_call_to_local_crate<'tcx>(tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> bool {
+ tcx.is_foreign_item(instance.def_id())
+ && tcx.exported_non_generic_symbols(LOCAL_CRATE).iter().any(|(sym, _info)| {
+ sym.symbol_name_for_local_instance(tcx) == tcx.symbol_name(instance)
+ })
+ }
+
let def_id = instance.def_id();
!def_id.is_local()
&& tcx.is_compiler_builtins(LOCAL_CRATE)
&& !is_llvm_intrinsic(tcx, def_id)
&& !tcx.should_codegen_locally(instance)
+ && !is_extern_call_to_local_crate(tcx, instance)
}
impl CrateInfo {The linear search of all exported non-generic symbols seems bad. It's only done a handful of times so it doesn't have a visible impact on compile time of compiler-builtins... yet. |
Member
|
@rustbot author |
rustbot
removed
the
S-waiting-on-review
Collaborator
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
This was referenced Apr 16, 2026
folkertdev
force-pushed
the
float-intrinsic-fallback
branch
from
90b9d6a to
176fd06
Compare
Collaborator
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
A number of float operations from libm have intrinsics for optimization, but it is also okay to just call the libm functions directly. Add a fallback for these cases, including converting to/from another float size where needed, so the backends don't need to override these.
in `is_call_from_compiler_builtins_to_upstream_monomorphization`, suggested by Seathlin
tgross35
force-pushed
the
float-intrinsic-fallback
branch
from
69882f0 to
21d67bf
Compare
This comment has been minimized.
This comment has been minimized.
Contributor
|
@RalfJung we're running into We could manually disable the failing tests but maybe a list of "never use the fallback for these" intrinsics would be better? or some sort of attribute? |
Collaborator
|
cc @rust-lang/miri |
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Member
|
I don't think those would be better than the Can you make it so it only skips the nondet tests rather than skipping the entire test? Also please add comments explaining why this gets skipped. |
RalfJung
reviewed
Jun 24, 2026
| pub fn sqrtf16(x: f16) -> f16; | ||
| pub fn sqrtf16(x: f16) -> f16 { | ||
| libm::sqrtf16(x) | ||
| } |
Member
There was a problem hiding this comment.
Adding a fallback body now will make it harder to constify these intrinsics later due to #150961. So -- is that really worth it? At least for intrinsics that we already have softfloat impls for, maybe it'd be better to not have fallback bodies.
Contributor
Well, it's tricky to deal with the errors in the tokio tests from They assert on the output too, so we can't just skip the test. We can fake the output? Idk this just seems really fragile. |
Member
|
Ah, yeah that's annoying. Can we have something in the rustc test suite that tests the fallback bodies? That might be more robust / easier to maintain than doing that via Miri. |
Contributor
|
Well we can add And then some sort of |
Member
Yeah something like that -- but it may need its own sysroot build.
What purpose does that serve? We already default to requiring a Miri override for every intrinsic that does not have I added this Miri intrinsic fallback body test to let us catch bugs in the fallback intrinsics as some of those have complicated bodies. We should ensure we have test coverage for the stuff in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
View all comments
A number of float operations from libm have intrinsics for optimization, but it is also okay to just call the libm functions directly. Add a fallback for these cases, including converting to/from another float size where needed, so the backends don't need to override these.
r? @RalfJung