Skip to content

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Nov 18, 2021

IEEE 754-2019 removed the minNum (min in Rust) and maxNum (max in Rust) operations in favor of the newly created minimum and maximum operations due to their non-associativity that cannot be fix in a backwards compatible manner. This PR adds fN::{minimun,maximum} functions following the new rules.

IEEE 754-2019 Rules

minimum(x, y) is x if x < y, y if y < x, and a quiet NaN if either operand is a NaN, according to 6.2.
For this operation, −0 compares less than +0. Otherwise (i.e., when x = y and signs are the same)
it is either x or y.

maximum(x, y) is x if x > y, y if y > x, and a quiet NaN if either operand is a NaN, according to 6.2.
For this operation, +0 compares greater than −0. Otherwise (i.e., when x = y and signs are the
same) it is either x or y.

"IEEE Standard for Floating-Point Arithmetic," in IEEE Std 754-2019 (Revision of IEEE 754-2008) , vol., no., pp.1-84, 22 July 2019, doi: 10.1109/IEEESTD.2019.8766229.

Implementation

This implementation is inspired by the one in glibc (it self derived from the C2X draft) expect that:

  • it doesn't use copysign because it's not available in core and also because copysign is unnecessary (we only want to check the sign, no need to create a new float)
  • it also prefer other > self instead of self < other like IEEE 754-2019 does

I originally tried to implement them using intrinsics but LLVM error out when trying to lower them to machine intructions, GCC doesn't yet have built-ins for them, only cranelift support them nativelly (as it doesn't support the nativelly the old sementics).

Helps with #83984

@rust-highfive
Copy link
Contributor

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 18, 2021
@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Nov 18, 2021
Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

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

This makes sense to offer to me -- it's clearly useful, and llvm having an intrinsic for it is extra evidence that it makes sense in core, even if it's not implemented yet. I'm not on libs-api, but I'd be fine putting this in as unstable.

Can you open a tracking issue for this and add the number to the stability attributes?

@Urgau Urgau force-pushed the float-minimum-maximum branch from 1645950 to 2bad893 Compare November 20, 2021 09:15
@Urgau
Copy link
Member Author

Urgau commented Nov 20, 2021

This makes sense to offer to me -- it's clearly useful, and llvm having an intrinsic for it is extra evidence that it makes sense in core, even if it's not implemented yet. I'm not on libs-api, but I'd be fine putting this in as unstable.

👍 Thanks.

Can you open a tracking issue for this and add the number to the stability attributes?

Done. I also squash some of the commits.

@scottmcm
Copy link
Member

Thanks for the PR! This is reversible, so I feel ok approving for nightly without bothering libs-api.

r? @scottmcm
@bors r+

@bors
Copy link
Collaborator

bors commented Nov 20, 2021

📌 Commit e2ec3b1 has been approved by scottmcm

@rust-highfive rust-highfive assigned scottmcm and unassigned dtolnay Nov 20, 2021
@bors 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 20, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 21, 2021
…askrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#91008 (Adds IEEE 754-2019 minimun and maximum functions for f32/f64)
 - rust-lang#91070 (Make `LLVMRustGetOrInsertGlobal` always return a `GlobalVariable`)
 - rust-lang#91097 (Add spaces in opaque `impl Trait` with more than one trait)
 - rust-lang#91098 (Don't suggest certain fixups (`.field`, `.await`, etc) when reporting errors while matching on arrays )

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 789d168 into rust-lang:master Nov 21, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

9 participants