Always inline functions signatures containing `f16` or `f128` #133050

tgross35 · 2024-11-14T20:19:02Z

There are a handful of tier 2 and tier 3 targets that cause a LLVM crash or linker error when generating code that contains f16 or f128. The cranelift backend also does not support these types. To work around this, every function in std or core that contains these types must be marked #[inline] in order to avoid sending any code to the backend unless specifically requested.

However, this is inconvenient and easy to forget. Introduce a check for these types in the frontend that automatically inlines any function signatures that take or return f16 or f128.

Note that this is not a perfect fix because it does not account for the types being passed by reference or as members of aggregate types, but this is sufficient for what is currently needed in the standard library.

Fixes: #133035
Closes: #133037

rustbot · 2024-11-14T20:19:10Z

r? @compiler-errors

rustbot has assigned @compiler-errors.
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 · 2024-11-14T20:19:12Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

tgross35 · 2024-11-14T20:24:39Z

I'm not sure what I am missing here - testing by setting #[inline(always)] the codegen tests seem to emit code (maybe a compiletest option). Aside from that, I don't think generics or traits are handled at this point.

r? @saethlin, I could use a bit of help here

tgross35 · 2024-11-14T20:35:34Z

Oh right, no_mangle forces codegen

saethlin · 2024-11-14T20:36:39Z

You're getting code emitted because the functions are #[no_mangle], which is the first thing that cross_crate_inlinable checks for. contains_extern_indicator is perhaps poorly named.

You might also run into

rust/compiler/rustc_middle/src/mir/mono.rs

Lines 107 to 113 in c82e0df

    
           let generate_cgu_internal_copies = tcx 
        
               .sess 
        
               .opts 
        
               .unstable_opts 
        
               .inline_in_all_cgus 
        
               .unwrap_or_else(|| tcx.sess.opts.optimize != OptLevel::No) 
        
               && !tcx.sess.link_dead_code();

after fixing that.

RalfJung · 2024-11-14T21:20:10Z

compiler/rustc_mir_transform/src/cross_crate_inline.rs

+    let sig = tcx.fn_sig(def_id).instantiate_identity();
+    for ty in sig.inputs().skip_binder().iter().chain(std::iter::once(&sig.output().skip_binder()))
+    {
+        // FIXME(f16_f128): in order to avoid crashes building `core`, always inline to skip
+        // codegen if the function is not used.


if you move this down after let mir, you can just iterate over mir.args_iter() instead of having to call fn_sig

I think this would require moving let mir above the checks against opts.incremental, OptLevel::No, and threashold to avoid hitting a return false before we check for the types. Would this wind up with a perf impact since we don't generate mir here in these cases?

Hm, I thought having this check so far down would be fine. But maybe not.

Since this is a (temporary, right?) hack, I'd rather the diff be small and localized than pretty. What you've written here is good.

(temporary, right?)

Yes please, I don't think we will be able to stabilize these types until LLVM at least doesn't crash on everything T2+.

RalfJung · 2024-11-14T21:22:07Z

tests/codegen/float/f16-f128-inline.rs

+
+pub fn f16_arg(_a: f16) {
+    // CHECK-NOT: f16_arg
+    todo!()


Are you sure the automatic inlining heuristics wouldn't anyway already mark these functions as inlineable? Might be worth setting cross_crate_inline_threshold to 0 to be sure that doesn't mask the test.

I added a revision with -Zcross-crate-inline-threshold=never -Zmir-opt-level=0 -Cno-prepopulate-passes, is that reasonable?

You should also add -Copt-level=0.

Oh yeah of course. Done, tests still pass.

-Cno-prepopulate-passes just disables LLVM optimizations, rustc changes its behavior in a few places based on the value of opt-level.

tgross35 · 2024-11-14T21:27:57Z

Is there a good way to walk the signature and check for the types recursively at this point? I'm realizing that &f16/&f128 aren't matched, it seems better to not special case them.

You might also run into

rust/compiler/rustc_middle/src/mir/mono.rs

Lines 107 to 113 in c82e0df

let generate_cgu_internal_copies = tcx

.sess

.opts

.unstable_opts

.inline_in_all_cgus

.unwrap_or_else(|| tcx.sess.opts.optimize != OptLevel::No)

&& !tcx.sess.link_dead_code();

after fixing that.

Should this function be updated to emit a LocalCopy if generate_cgu_internal_copies || /* contains f16/f128 */? I think this relates to Ralf's comment.

RalfJung · 2024-11-14T21:29:36Z

I'm realizing that &f16/&f128 aren't matched, it seems better to not special case them.

Please change the const_assert to not use references, that was just a work-around.

tgross35 · 2024-11-14T21:57:09Z

I need to unjunkify my commit history (edit: done) but as-is, this resolves the problem at #133035.

These types are currently passed by reference, which does not avoid the backend crashes. Change these back to being passed by value, which makes the types easier to detect for automatic inlining.

There are a handful of tier 2 and tier 3 targets that cause a LLVM crash or linker error when generating code that contains `f16` or `f128`. The cranelift backend also does not support these types. To work around this, every function in `std` or `core` that contains these types must be marked `#[inline]` in order to avoid sending any code to the backend unless specifically requested. However, this is inconvenient and easy to forget. Introduce a check for these types in the frontend that automatically inlines any function signatures that take or return `f16` or `f128`. Note that this is not a perfect fix because it does not account for the types being passed by reference or as members of aggregate types, but this is sufficient for what is currently needed in the standard library. Fixes: rust-lang#133035 Closes: rust-lang#133037

saethlin · 2024-11-14T22:55:30Z

Should this function be updated to emit a LocalCopy if generate_cgu_internal_copies || /* contains f16/f128 */? I think this relates to Ralf's comment.

After thinking about this I think I've convinced myself that no such change is required, because that code is only run after MirUsedCollector has determined that the Instance must be codegenned. The choice of LocalCopy or GloballyShared there only changes how we're going to codegen the instance, not whether we are going to.

saethlin · 2024-11-14T23:07:12Z

I've manually verified that

cargo +stage1 build -Zbuild-std=core --target=powerpc64-ibm-aix
cargo +stage1 build -Zbuild-std=core --target=arm64ec-pc-windows-msvc

Work on this PR, and not on nightly.

@bors r+

bors · 2024-11-14T23:07:14Z

📌 Commit 5d81891 has been approved by saethlin

It is now in the queue for this repository.

Always inline functions signatures containing `f16` or `f128` There are a handful of tier 2 and tier 3 targets that cause a LLVM crash or linker error when generating code that contains `f16` or `f128`. The cranelift backend also does not support these types. To work around this, every function in `std` or `core` that contains these types must be marked `#[inline]` in order to avoid sending any code to the backend unless specifically requested. However, this is inconvenient and easy to forget. Introduce a check for these types in the frontend that automatically inlines any function signatures that take or return `f16` or `f128`. Note that this is not a perfect fix because it does not account for the types being passed by reference or as members of aggregate types, but this is sufficient for what is currently needed in the standard library. Fixes: rust-lang#133035 Closes: rust-lang#133037

rustbot assigned compiler-errors Nov 14, 2024

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. labels Nov 14, 2024

tgross35 marked this pull request as draft November 14, 2024 20:20

tgross35 mentioned this pull request Nov 14, 2024

Revert #132662 to fix regression on targets without f128 support #133037

Open

rustbot assigned saethlin and unassigned compiler-errors Nov 14, 2024

RalfJung reviewed Nov 14, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

tgross35 marked this pull request as ready for review November 14, 2024 21:57

tgross35 changed the title ~~[wip] Always inline f16 and f128~~ Always inline f16 and f128 Nov 14, 2024

Pass f16 and f128 by value in const_assert!

b77dbbd

These types are currently passed by reference, which does not avoid the backend crashes. Change these back to being passed by value, which makes the types easier to detect for automatic inlining.

tgross35 force-pushed the inline-f16-f128 branch from 53a873b to 781ea18 Compare November 14, 2024 22:09

tgross35 changed the title ~~Always inline f16 and f128~~ Always inline functions signatures containing f16 or f128 Nov 14, 2024

tgross35 force-pushed the inline-f16-f128 branch from 781ea18 to 5d81891 Compare November 14, 2024 22:24

tgross35 added the F-f16_and_f128 `#![feature(f16)]`, `#![feature(f128)]` label Nov 14, 2024

tgross35 mentioned this pull request Nov 14, 2024

feat: add aio for solarish rust-lang/libc#4033

Open

3 tasks

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 14, 2024

workingjubilee mentioned this pull request Nov 15, 2024

Rollup of 8 pull requests #133059

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always inline functions signatures containing `f16` or `f128` #133050

Always inline functions signatures containing `f16` or `f128` #133050

tgross35 commented Nov 14, 2024 •

edited

Loading

rustbot commented Nov 14, 2024

rustbot commented Nov 14, 2024

tgross35 commented Nov 14, 2024

tgross35 commented Nov 14, 2024

saethlin commented Nov 14, 2024

RalfJung Nov 14, 2024

tgross35 Nov 14, 2024

RalfJung Nov 14, 2024

saethlin Nov 14, 2024

tgross35 Nov 14, 2024

RalfJung Nov 14, 2024

tgross35 Nov 14, 2024

saethlin Nov 14, 2024 •

edited

Loading

tgross35 Nov 14, 2024

saethlin Nov 14, 2024

This comment has been minimized.

tgross35 commented Nov 14, 2024

RalfJung commented Nov 14, 2024

tgross35 commented Nov 14, 2024 •

edited

Loading

saethlin commented Nov 14, 2024

saethlin commented Nov 14, 2024

bors commented Nov 14, 2024

Always inline functions signatures containing f16 or f128 #133050

Are you sure you want to change the base?

Always inline functions signatures containing f16 or f128 #133050

Conversation

tgross35 commented Nov 14, 2024 • edited Loading

rustbot commented Nov 14, 2024

rustbot commented Nov 14, 2024

tgross35 commented Nov 14, 2024

tgross35 commented Nov 14, 2024

saethlin commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

tgross35 commented Nov 14, 2024

RalfJung commented Nov 14, 2024

tgross35 commented Nov 14, 2024 • edited Loading

saethlin commented Nov 14, 2024

saethlin commented Nov 14, 2024

bors commented Nov 14, 2024

Always inline functions signatures containing `f16` or `f128` #133050

Always inline functions signatures containing `f16` or `f128` #133050

tgross35 commented Nov 14, 2024 •

edited

Loading

saethlin Nov 14, 2024 •

edited

Loading

tgross35 commented Nov 14, 2024 •

edited

Loading