Use intrinsics for `{f16,f32,f64,f128}::{minimum,maximum}` operations #140792

Urgau · 2025-05-08T10:56:07Z

This PR creates intrinsics for {f16,f32,f64,f64}::{minimum,maximum} operations.

This wasn't done when those operations were added as the LLVM support was too weak but now that LLVM has libcalls for unsupported platforms we can finally use them.

Cranelift and GCC¹ support are partial, Cranelift doesn't support f16 and f128, while GCC doesn't support f16.

r? @tgross35

try-job: aarch64-gnu
try-job: dist-various-1
try-job: dist-various-2

https://www.gnu.org/software///gnulib/manual/html_node/Functions-in-_003cmath_002eh_003e.html ↩

rustbot · 2025-05-08T10:56:13Z

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

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

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

compiler/rustc_const_eval/src/interpret/intrinsics.rs

library/core/src/intrinsics/mod.rs

RalfJung · 2025-05-08T12:11:48Z

library/core/src/num/f16.rs

+            if self > other {
+                self
+            } else if other > self {
+                other
+            } else if self == other {
+                if self.is_sign_positive() && other.is_sign_negative() { self } else { other }
+            } else {
+                self + other
+            }


Would it be worth preserving this as "fallback" implementation (by giving the intrinsic a body)? Then const-eval wouldn't need an implementation at all, and also cranelift and GCC would not need any extra work.

We could, but it's not currently done min and max, @tgross35 was that intentional?

min and max are way older than our ability to have fallback bodies.

Yes, but as part of adding f16 and f128 we could have added a fallback, but it wasn't done. Hence my question if it was intentional that fallback bodies haven't been added.

Also the minnum16 and minnum128 intrinsics are quite recent, idk if we had already had fallback body.

Even the f16/f128 ones might be older than fallback bodies. Also they were obviously just made to be consistent with the f32/f64 ones (hence the terrible name...).

I have added a fallback for {minimum,maximum}{16,128}, GCC and Cranelift should have full support.

I didn't touched const-eval as we have the necessary support in rustc_apfloat.

We could, but it's not currently done min and max, @tgross35 was that intentional?

To follow up, when I did the original implementation I tried to keep things as similar as possible to the existing float types. Maybe it makes sense to update those later?

Yeah, the more things that can have fallback MIR the better -- it means they work for out-of-tree backends too, for example. (And might help tools like kani too, to not have to manually handle them in verifiers.)

As was mentioned, it's just that for a long time it wasn't possible to give things fallback MIR, so there's a big backlog.

compiler/rustc_codegen_gcc/src/intrinsic/mod.rs

tgross35 · 2025-05-09T12:12:10Z

library/core/src/num/f16.rs

+            if self > other {
+                self
+            } else if other > self {
+                other
+            } else if self == other {
+                if self.is_sign_positive() && other.is_sign_negative() { self } else { other }
+            } else {
+                self + other
+            }


We could, but it's not currently done min and max, @tgross35 was that intentional?

To follow up, when I did the original implementation I tried to keep things as similar as possible to the existing float types. Maybe it makes sense to update those later?

library/core/src/num/f128.rs

compiler/rustc_codegen_gcc/src/intrinsic/mod.rs

library/core/src/intrinsics/mod.rs

RalfJung · 2025-05-09T13:34:48Z

@rust-lang/lang Never mind, turns out the functions are not actually stable, so we definitely don't need an FCP then. :)

compiler/rustc_codegen_gcc/src/intrinsic/mod.rs

library/core/src/num/f32.rs

matthiaskrgr · 2025-05-10T14:25:31Z

@bors r-
#140890 (comment)

Urgau · 2025-05-10T15:37:20Z

@bors try

bors · 2025-05-10T15:38:31Z

⌛ Trying commit 7f0ae5e with merge b8a7222...

bors · 2025-05-10T17:35:29Z

☀️ Try build successful - checks-actions
Build commit: b8a7222 (b8a72228198170e8c3dc3df531635c7bf11c92ae)

Urgau · 2025-05-10T17:45:35Z

It appears that on architecture where LLVM implements custom lowering for llvm.minimum.* and llvm.maximum.*, mainly x86 and aarch64, there a SelectionDAG error on f128 (and only this one). I've opened issues about this on LLVM repo: llvm/llvm-project#139380 and llvm/llvm-project#139381.

I've disabled the lowering to llvm.minimum.f128 and llvm.maximum.f128 for every architectures and instead fallback to our intrinsic fallback. Which is passing CI (the try build above).

@bors r=scottmcm,traviscross,tgross35

bors · 2025-05-10T17:45:38Z

📌 Commit 7f0ae5e has been approved by scottmcm,traviscross,tgross35

It is now in the queue for this repository.

rustbot assigned tgross35 May 8, 2025

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 May 8, 2025

This comment has been minimized.

Sign in to view

Urgau force-pushed the minimum-maximum-intrinsics branch from 360cdaf to f308845 Compare May 8, 2025 11:09

RalfJung reviewed May 8, 2025

View reviewed changes

compiler/rustc_const_eval/src/interpret/intrinsics.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 8, 2025

View reviewed changes

library/core/src/intrinsics/mod.rs Show resolved Hide resolved

RalfJung reviewed May 8, 2025

View reviewed changes

antoyo reviewed May 8, 2025

View reviewed changes

compiler/rustc_codegen_gcc/src/intrinsic/mod.rs Outdated Show resolved Hide resolved

Urgau force-pushed the minimum-maximum-intrinsics branch from f308845 to 8b600e5 Compare May 8, 2025 13:39

Urgau changed the title ~~Use intrinsics for {f16,f32,f64,f64}::{minimum,maximum} operations~~ Use intrinsics for {f16,f32,f64,f128}::{minimum,maximum} operations May 8, 2025

tgross35 reviewed May 9, 2025

View reviewed changes

RalfJung reviewed May 9, 2025

View reviewed changes

library/core/src/intrinsics/mod.rs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

RalfJung added T-lang Relevant to the language team I-lang-nominated Nominated for discussion during a lang team meeting. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination labels May 9, 2025

RalfJung removed T-lang Relevant to the language team I-lang-nominated Nominated for discussion during a lang team meeting. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination labels May 9, 2025

Urgau force-pushed the minimum-maximum-intrinsics branch from 96b7a3d to f63c9d7 Compare May 9, 2025 13:44

antoyo reviewed May 9, 2025

View reviewed changes

compiler/rustc_codegen_gcc/src/intrinsic/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 9, 2025

View reviewed changes

library/core/src/num/f32.rs Outdated Show resolved Hide resolved

Urgau force-pushed the minimum-maximum-intrinsics branch from f63c9d7 to 7d53e31 Compare May 9, 2025 14:25

This comment has been minimized.

Sign in to view

Urgau force-pushed the minimum-maximum-intrinsics branch from 7d53e31 to ca9dfb2 Compare May 9, 2025 14:48

This comment has been minimized.

Sign in to view

matthiaskrgr mentioned this pull request May 10, 2025

Rollup of 8 pull requests #140890

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 10, 2025

Use the fallback body for {minimum,maximum}f128 on LLVM as well.

7f0ae5e

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 10, 2025

RalfJung mentioned this pull request May 10, 2025

test intrinsic fallback bodies with Miri #140903

Merged

fmease mentioned this pull request May 11, 2025

Rollup of 7 pull requests #140912

Merged

bors merged commit 3c8950c into rust-lang:master May 11, 2025
7 checks passed

rustbot added this to the 1.89.0 milestone May 11, 2025

tgross35 mentioned this pull request May 17, 2025

Assertion error on arm-unknown-linux-gnueabihf for maximum/minimum float #141087

Closed

Urgau mentioned this pull request Feb 25, 2025

Tracking Issue for float_minimum_maximum #91079

Open

4 tasks

Uh oh!

Use intrinsics for {f16,f32,f64,f128}::{minimum,maximum} operations #140792

Use intrinsics for {f16,f32,f64,f128}::{minimum,maximum} operations #140792

Uh oh!

Conversation

Urgau commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

rustbot commented May 8, 2025

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

RalfJung May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Urgau May 8, 2025

Choose a reason for hiding this comment

Uh oh!

RalfJung May 8, 2025

Choose a reason for hiding this comment

Uh oh!

Urgau May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Urgau May 8, 2025

Choose a reason for hiding this comment

Uh oh!

tgross35 May 9, 2025

Choose a reason for hiding this comment

Uh oh!

scottmcm May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tgross35 May 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

RalfJung commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

matthiaskrgr commented May 10, 2025

Uh oh!

Urgau commented May 10, 2025

Uh oh!

bors commented May 10, 2025

Uh oh!

bors commented May 10, 2025

Uh oh!

Urgau commented May 10, 2025

Uh oh!

bors commented May 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Use intrinsics for `{f16,f32,f64,f128}::{minimum,maximum}` operations #140792

Use intrinsics for `{f16,f32,f64,f128}::{minimum,maximum}` operations #140792

Urgau commented May 8, 2025 •

edited

Loading

RalfJung May 8, 2025 •

edited

Loading

Urgau May 8, 2025 •

edited

Loading

RalfJung May 8, 2025 •

edited

Loading

scottmcm May 9, 2025 •

edited

Loading

RalfJung commented May 9, 2025 •

edited

Loading