intrinsics: Add a fallback for non-const libm float functions

[tgross35]

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

[rustbot]

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

[tgross35]

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.

[RalfJung]

I don't think there is -- please file an issue.

[tgross35]

Opened #150961

[tgross35]

error: `compiler_builtins` cannot call functions through upstream monomorphizations; encountered invalid call from `core::intrinsics::sqrtf64` to `core::num::libm::sqrt`
    --> library/core/src/intrinsics/mod.rs:1047:1
     |
1047 | pub fn sqrtf64(x: f64) -> f64 {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Huh - so #[inline] doesn't make a difference here. The body itself shouldn't be a problem since it's calling an extern function. @saethlin any idea what is going on here?

[saethlin]

At a glance, your reasoning is completely backwards. The fallback body for sqrtf64 is being monomorphized, which is disallowed because it calls a libm function that is upstream.

[tgross35]

It shouldn't be; the libm module in core is only bindings to libm, the function call is just an extern.

[RalfJung]

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. ;)
Is there anyone else who knows those details of libm and our compiler-builtins? If not, I'm also fine with approving this based on your judgment.

[tgross35]

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 likely_available match that. Then the remaining maybe_available symbols are part of glibc (e.g. https://github.com/bminor/glibc/blob/e539a269990dac3ff4d2432c0eb6966a5ee4f274/sysdeps/unix/sysv/linux/i386/libm.abilist#L590), but not too much else.

[saethlin]

It shouldn't be; the libm module in core is only bindings to libm, the function call is just an extern.

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.

[tgross35]

It shouldn't be; the libm module in core is only bindings to libm, the function call is just an extern.

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.

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 sqrt symbol so it will be available to link. I can drop the sqrt fallbacks for now since the status quo is fine anyway, but it seems like maybe the ideal solution would be something like an #[allow_compiler_builtins_link] attribute on the extern "C" block?

It probably goes without saying but libm isn't calling the sqrt intrinsic in fn sqrt itself, it's used elsewhere so we get the asm lowering if available.

[saethlin]

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 should_codegen_locally... except that it is codegenned in the local crate in this one case because of the linkage relationship.

So the missing check is whether the Instance inside is_call_from_compiler_builtins_to_upstream_monomorphization is actually an extern declaration of a symbol name that is in the current crate's exported_symbols. I think this is a feasible check for you to write if you want. I'm a bit busy for a bit or I'd try to slap it together. @bjorn3 since this is fiddly linkage stuff.

[saethlin]

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.

[RalfJung]

@rustbot author

[rustbot]

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

[rust-bors]

☀️ Try build successful (CI)
Build commit: 5d627ac (5d627ac6cbb04a967207fff0e3b3afea15febf09)
Base parent: 07074cf (07074cfe1d991fe06b7fc613bbbd1dd9d0d37c80)

[folkertdev]

@bors r=RalfJung,saethlin

[rust-bors]

📌 Commit e732dd1 has been approved by RalfJung,saethlin

It is now in the queue for this repository.

[JonathanBrouwer]

@rust-timer build ba59afc

[rust-timer]

Finished benchmarking commit (ba59afc): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.0%, 0.2%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.0%	[-0.1%, -0.0%]	3
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.4%, secondary -0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	4.8%	[4.8%, 4.8%]	1
Improvements ✅ (primary)	-3.9%	[-4.3%, -3.6%]	2
Improvements ✅ (secondary)	-1.8%	[-3.1%, -1.1%]	3
All ❌✅ (primary)	-2.4%	[-4.3%, 0.7%]	3

Cycles

Results (primary 1.8%, secondary -1.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.8%	[1.8%, 1.8%]	1
Regressions ❌ (secondary)	4.4%	[2.2%, 8.1%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.6%	[-9.0%, -1.8%]	9
All ❌✅ (primary)	1.8%	[1.8%, 1.8%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 490.768s -> 490.048s (-0.15%)
Artifact size: 389.74 MiB -> 389.17 MiB (-0.15%)

[JonathanBrouwer]

@rustbot label: +perf-regression-triaged
Noise