Float min/max/clamp follow IEEE754-2008 (and not IEEE754-2019) WRT negative zeros

[thomcc]

In a blog post (shameless plug) I wrote on clamping between 0.0 and 1.0, I noticed that we don't treat -0.0 as less than +0.0. Instead, we return whichever is the first argument:

>> f32::min(0.0, -0.0)
0.0
>> f32::min(-0.0, 0.0)
-0.0
>> f32::max(0.0, -0.0)
0.0
>> f32::max(-0.0, 0.0)
-0.0

That behavior is kinda incoherent, probably unintentional, and seems highly unlikely that anybody is relying on it, so I suspect we're free to fix it.

While IEEE-754 defines two families of min/max functions (differing on NaN handling), all of them are required to treat -0.0 as less than +0.0. We should fix our version to do this ¹.

This problem is present in {float}::clamp too, which ideally would produce +0.0 for f32::clamp(-0.0, +0.0, _). (However, note that clamp uses a different² version of min/max than max/min, so fixing it here won't automatically fix it for clamp).

Excerpt from IEEE754 2019 section 9.6 "Minimum and maximum operations" (emphasis mine)

• 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. ...

• maximumNumber(x, y) is x if x > y, y if y > x, and the number if one operand is a number and the other is a NaN. For this operation, +0 compares greater than −0. ...

• 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. ...

• minimumNumber(x, y) is x if x < y, y if y < x, and the number if one operand is a number and the other is a NaN. For this operation, −0 compares less than +0. ...

CC @workingjubilee

---

¹ Alternatively, since these functions only "should" be provided, we could say that we just don't provide equivalents to those functions and that ours are different functions. If we do this, we should have a very good reason IMO, and should document this as well.

² Note that for reasons³ our {float}::clamp functions are documented as propagating NaNs, but {float}::{min, max} are not. This means this does have to be fixed in multiple places, although, it's coherent under IEEE754 if we assume

• {float}::{min, max} use minimumNumber and maximumNumber respectively
• {float}::clamp uses minimum and maximum

³ Nobody asked, but personally, I'd rather clamp be consistent with min/max, which for this has the benefit of guaranteeing that the result is always in the provided bound, which is quite desirable sometimes. I know it was probably deliberate, and I do get why, and know it can't be changed, but I'm still going to use this footnote-within-a-footnote on an obscure floating point bug report to gripe about it.

[eggyal]

@rustbot label: +A-floating-point +C-bug

[est31]

Rust just calls the llvm.minnum.f64 or llvm.minnum.f32 intrinsics. Quoting the language reference:

If either operand is a NaN, returns the other non-NaN operand. Returns NaN only if both operands are NaN. The returned NaN is always quiet. If the operands compare equal, returns a value that compares equal to both operands. This means that fmin(+/-0.0, +/-0.0) could return either -0.0 or 0.0.

There are also llvm.minimum.f64 and llvm.minimum.f32 intrinsics (needed to squint the first time, it's minnum vs minimum). Quoting its docs:

If either operand is a NaN, returns NaN. Otherwise returns the lesser of the two arguments. -0.0 is considered to be less than +0.0 for this intrinsic. Note that these are the semantics specified in the draft of IEEE 754-2018.

Maybe rustc should be changed to use minimum instead of minnum?

[eggyal]

Maybe rustc should be changed to use minimum instead of minnum?

The docs for f32::min and f64::min both state:

If one of the arguments is NaN, then the other argument is returned.

Which this proposed change would break.

[thomcc]

LLVM's minnum (at the link above) also says

Follows the IEEE-754 semantics for minNum, except for handling of signaling NaNs. This match’s the behavior of libm’s fmin.

which is incorrect for -0.0, what this bug is. So maybe this is an LLVM bug.

[est31]

More reading:

• llvm.minimum.* introduction commit from 2018
• llvm.minnum.* introduction commit from 2014

The minimum intrinsic seems to exist since LLVM 8.0, while the minimum LLVM that rustc supports is 10, so it should be safe to use from that point of view.

Which this proposed change would break.

Hmm that's bad. :/.

[thomcc]

I think this is a bug in llvm.minnum, although it's unclear which of

Follows the IEEE-754 semantics for minNum, except for handling of signaling NaNs

and

This means that fmin(+/-0.0, +/-0.0) could return either -0.0 or 0.0.

is more important (since those two parts of the docs are in contradiction). If there's a performance cost to the former, I could imagine LLVM would rather keep it as-is.

That said, for them it wouldn't be breaking to fix the bug so that they're in compliance with IEEE minNum, since they don't specify the result.

[est31]

For Rust, I don't know how much the documentation mentioning this behaviour prevents a change. It might just be that it only means that the documentation needs to be changed as well, nothing more.

The sentence seems to have been introduced by commit f44d287 .

[thomcc]

I filed an LLVM bug: https://bugs.llvm.org/show_bug.cgi?id=49886

And yeah, I just busted out my old copy of IEEE 754-2008 and can confirm that it doesn't specify the sign of -0.0. I mentioned maybe a new intrinsic could be added instead.

For Rust, I don't know how much the documentation mentioning this behaviour prevents a change. It might just be that it only means that the documentation needs to be changed as well, nothing more.

IMO it would be a breaking change (even if it weren't documented, but especially given that it is). Faking Ord for floats by a.partial_cmp(b).unwrap() isn't unheard of, and so changing things to introduce way more NaNs could cause working code to break. (Also, I could easily imagine using the documentation there to justify not checking for NaN in the result of f32::min/f32::max, and would be upset if it changed out from under me)

Also way more people will notice and care about the handling of NaNs than -0.0 also.

---

All that said, we can fix this for f32::clamp by using llvm.minimum and llvm.maximum instead, which isn't a breaking change (it propagates NaN, as mentioned).

[nagisa]

Do hardware implementations support this behaviour? Some random documentation for minsd of x86_64 says:

If the values being compared are both 0.0s (of either sign), the value in the second source operand is returned.

thus not supporting the behaviour desired here. AArch's FMIN appears to handle the 0.0 the "right" way.

---

LLVM's x86 backend is unable to select machine code for the llvm.minimum intrinsic, so its not usable for generic implementations either way.

[eggyal]

I looked at what cg_clif is doing:

rust/compiler/rustc_codegen_cranelift/src/intrinsics/mod.rs

Lines 1053 to 1062 in 673d0db

	minnumf32, (v a, v b) {
	let val = fx.bcx.ins().fmin(a, b);
	let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f32));
	ret.write_cvalue(fx, val);
	};
	minnumf64, (v a, v b) {
	let val = fx.bcx.ins().fmin(a, b);
	let val = CValue::by_val(val, fx.layout_of(fx.tcx.types.f64));
	ret.write_cvalue(fx, val);
	};

Taken together with Cranelift's documentation for its fmin and fmin_pseudo instructions, it would appear that cg_clif already takes the IEEE754 minimum approach (albeit there's an open issue that could see it switch to using minimumNumber instead)—and I don't think there's (currently) any Cranelift instruction available by which it could adopt the existing cg_llvm semantics.

To the extent that has any bearing on this discussion, I guess it just pushes slightly toward the same semantics being adopted in cg_llvm too.

IMO it would be a breaking change (even if it weren't documented, but especially given that it is). Faking Ord for floats by a.partial_cmp(b).unwrap() isn't unheard of, and so changing things to introduce way more NaNs could cause working code to break. (Also, I could easily imagine using the documentation there to justify not checking for NaN in the result of f32::min/f32::max, and would be upset if it changed out from under me)

I agree, but I wonder whether there's any way something like a Crater run could provide an indication of the extent to which these semantics are actually being relied upon (I can't see how—though it could at very least determine how many crates actually use the existing min methods). If the semantics were to change, it may be sufficient to lint uses of min with a warning to that effect (although that would mean either forever having to suppress the lint if so desired, or at some point it no longer warning by default).

If, as I suspect, the semantics of the existing methods cannot be changed, perhaps alternative methods might be added instead (eg minimum or similar), with a view to eventually deprecating the existing min methods in a future edition?

[eggyal]

Reproducing here @sunfishcode's helpful summary in https://github.com/bjorn3/rustc_codegen_cranelift/issues/1049#issuecomment-817067277:

Rust+llvm are following the family of functions known in IEEE 754-2008 as minNum which prefer a number over a NaN and don't define what happens between -0.0 and +0.0 (and has surprising behavior on sNaN).

Cranelift's fmin follows IEEE 754-2019's new minimum and WebAssembly's min, which prefer a NaN over a number and treat -0.0 as less than +0.0 (and have unsurprising behavior on sNaN).

IEEE 754-2019 also defines minimumNumber, which prefers a number over a NaN and treats -0.0 as less than +0.0 (and has unsurprising behavior on sNaN). WebAssembly's MVP was designed before IEEE 754-2019's direction on this issue was known, so it didn't originally add operators for minimumNumber and maximumNumber. Now that IEEE 754-2019 is published, it's a reasonable assumption that WebAssembly should eventually add operators for these.

[thomcc]

I agree, but I wonder whether there's any way something like a Crater run could provide an indication of the extent to which these semantics are actually being relied upon

I'm certain I've implicitly relied on this in ways that wouldn't be tested on crater. A lot of the times you don't bother testing with NaN. Often, even if you do, it can be tricky to get NaN all the way into the right place in the code, but you want it handled a certain way if it does get there. Also, floating point stuff is common in gui and graphics code, some of which isn't open source, and the stuff that is doesn't run on crater (needs a GUI).

Again, I strongly feel that the documentation there is a promise that these are the semantics of the function, and that you can rely on them. Just because NaN is a weird value, and floats are less precise than the integers, doesn't mean the documented behavior of a function like this is allowed to change on that value. It's hard enough to handle floating point edge cases robustly.

If it wasn't floats and NaN, would you consider changing the behavior? Say of i32::signum on zero (which is an example of a case where the result isn't exactly "obvious", since the input isn't precisely part of the obvious input range) — the answer is (hopefully) of course not, the function promises that it behaves that way on the function, so it can't change.

Sorry, I might have a bit of a chip on my shoulder about this stuff, but I think it's a bit ridiculous to even consider, given that our IEEE compliance around NaN isn't even in question — we're compliant so long as this function is minimumNumber not minimum. It's the handling of negative zero where we're wrong, and we shouldn't break a behavioral promise the API makes for NaN (which people will notice) just to bring -0 (which very few will ever notice) into line.

And I'd only want to deprecate or lint if we can't bring it into line with minimumNumber, and even then, it'd be a very touch decision since these functions are very highly used, -0 is such an edge case, and there would be a lot of churn for such a small thing. A new function that implements minimum as speced by IEEE754 2019 would probably be fine though, if a bit confusing, given that we're getting total comparison stuff soon too.

[thomcc]

Anyway, changing the behavior on NaN could lead to UB: Something like x.max(0.0).min(255.0).to_int_unchecked::<u8>() is sound today on NaN input with the current rules, and becomes UB if max/min return NaN when given NaN input.

(Note: I don't think this example is that implausible)

[nikic]

Just so we're absolutely clear on this: The current behavior regarding negative zero in both Rust and LLVM is very much intentional, and should very much not be "fixed".

[thomcc]

The current behavior around negative zero is that it's unspecified. It follows an old version of the spec, but that function was replaced with one that clarifies the behavior for negative zero.

For LLVM, I have way weaker opinions, but if anything I think the "fix" is probably a new pair of intrinsics that match minimumNumber/maximumNumber. The current llvm.minnum/llvm.maxnum would stay as-is.

For Rust, I think it should be updated to be compliant with the new IEEE754 spec, which has all minimum and maximum functions treat -0.0 as less than 0.0, and removed the min/max functions where they're treated as equal. Do you have a reason that the current behavior around that should be kept?

[thomcc]

The current behavior {f32,f64}::{min,max}(-0.0, 0.0) is to just return the first argument, which seems incoherent, hard to rely on, likely platform dependent, and entirely undocumented. So I think we're free to change it.

If it turns out it's a big performance hit to handle it as specified, that could make a difference in my opinion though, perhaps.

If we don't change it, we should document that it's based on minNum from IEEE754-2008 (and not minimumNumber from IEEE754-2019), to avoid future confusion.

I also think there's no reason not to change clamp to just use llvm.minimum and llvm.maximum as-is, since it's compatible with their NaN handling, and that would fix this bug for it.

[nikic]

For Rust, I think it should be updated to be compliant with the new IEEE754 spec, which has all minimum and maximum functions treat -0.0 as less than 0.0, and removed the min/max functions where they're treated as equal. Do you have a reason that the current behavior around that should be kept?

The behavior exists for performance reasons. x86 does not support IEEE754 min/max operations, so all special cases need to be implemented explicitly. When targeting newer x86, NaN is handled through an additional cmpunord+blendv. On older targets (and if non-NaN cannot be proven) it's significantly more expensive. Handling negative zero adds more cost on top of that.

In addition to that, fmin/fmax in C are spec'd to follow IEEE754-2008 semantics, in that handling of negative zero is explicitly specified as optional for implementation feasibility reasons. This means that for soft-float targets, if the operation gets libcall-legalized, we do not have a guarantee that the invoked libcall will have special treatment of negative zero.

[thomcc]

Hmm, it's hard for me to say. Given that IEEE 754 2019 defines several min/max functions, but they all have the same behavior for -0.0 (that it's less than +0.0), that feels like a strong indication that that behavior is to be preferred.

I also feel like these functions are typically outside the hot path, and that in practice the branch that's needed to check for -0.0 will be correctly predicted essentially every time (assuming its a branch — and I guess it may not be). That said, the decision here would presumably be mirrored by core_simd, and I do feel that it's very plausible that a simd version of min/max could be in the hot path — I've written code where this is the case, and while the scalar fallback in that code didn't use f32::min/max (nor could it have, really, it needed to use an explicit comparison)...

And I guess there are probably cases I'm not thinking of where you need the minimum value in an array or something, and then f32::min would be in the hot path.

That said, I was surprised that we didn't follow IEEE754 2019, and someone else may be too in the future. I'd feel a bit better if there's an explicit decision here, since it's a decision to stick with an outdated version of these APIs for performance reasons...

[workingjubilee]

Regarding min/max, it sounds like NaN handling requires additional instructions, regardless of which behavior is desired on x86, and so that does not sound like a particularly strong performance concern.
It does sound like -0.0 vs. 0.0 is hypothetically a performance concern.
Rust has much bigger issues though re: performance concerns and floats.

[scottmcm]

I know it was probably deliberate

It was definitely deliberate, because there's no good choice for what non-NAN value to return for a NAN input.

[kornelski]

I also feel like these functions are typically outside the hot path

In image processing these functions are very often in the hot path, and -0 never happens.

Sadly, with float functions there's always this tension between prioritizing accuracy and performance. I know for some scientific cases it's necessary to handle every weird edge case up to ULP. OTOH there are lots of cases where floats are just used for banal imprecise arithmetic. Not because floats are the right tool for the job, but only because otherwise half of the CPU would sit unused.

[workingjubilee]

Most modern application processor architectures I have seen the pipeline diagrams for actually have their pipelines combined for floating point operations and SIMD, because they have combined floating point and SIMD registers, so integral SIMD math would use the same pipelines, and may actually outstrip floating point performance. Floating point performance on its own does matter, of course, which is why I think having a highly consistent semantics is not merely some academic or scientific mathematical concern, as it enables the fastest evaluation of all: the one taking zero time, because it was already calculated.

So while I am sympathetic to the performance concerns, I do not believe we should ignore the possibilities for alternatives, other gains we could be getting, or places we can make up for any losses we might encounter as a result of tightening our specification.

That said, I am not in any particular rush to a conclusion, here. I expect this issue, like many floating point issues in this repository, will remain open for some time before it resolves one way or another. I am merely noting that it is possible there is a solution that may satisfy both concerns.

[bjorn3]

Taken together with Cranelift's documentation for its fmin and fmin_pseudo instructions, it would appear that cg_clif already takes the IEEE754 minimum approach (albeit there's an open issue that could see it switch to using minimumNumber instead)—and I don't think there's (currently) any Cranelift instruction available by which it could adopt the existing cg_llvm semantics.

I have now adopted cg_llvm's semantics in bjorn3/rustc_codegen_cranelift@c6564f8. I would slightly prefer if either the semantics of Cranelift's fmin/fmax or fmin_pseudo/fmax_pseudo instructions would be chosen, but for now this fixes some libcore and stdsimd tests. In the later case by ensuring that the scalar min/max operations match the vector ones which already had the same semantics as cg_llvm due to explicitly using comparisons and lane selection instead of dedicated intrinsics.

[vinc17fr]

In image processing these functions are very often in the hot path, and -0 never happens.

@kornelski Note that if some code does something equivalent to y = x - x, then -y, this will give a −0. Said that, it is likely that in almost all cases, the visible behavior after the subsequent operations will be equivalent to +0.

Concerning the min and max function, if the compiler can determine that the (+0,−0) or (−0,+0) case cannot occur, it can optimize. Ditto if the programmer gives such a hint to the compiler.

[scottmcm]

As an example why fNN::max definitely can't be changed: doing .fold(f32::NAN, f32::max) currently works great to get the largest non-NaN in an iterator (or to get NAN if it's empty or all NANs), and I would consider it unacceptable to break that.

[thomcc]

Yeah, I find minNum/maxNum's behavior regarding nan's pretty good, although the arguments they make for why they changed it in 2019 are compelling as well. Changes here would need to be new functions, I think.

[scottmcm]

Honestly I like maximum/minimum semantics better, since it's the same kind of lift https://internals.rust-lang.org/t/lifting-binary-operations-e-g-t-t-option-t-option-t/6618/3?u=scottmcm that the other operators (like +) do. But minNum/maxNum aren't useless enough to even deprecate, let along change or remove.

[thomcc]

Right, i think there's cases where you want to propagate nans, and cases where you want to swallow them, and propagation is more consistent with most of floating point semantics.

[EdorianDark]

To define Clamp in terms of Minimum and Maximum instead of Min and Max is not possible right?
If it cannot changed, then I would add to Clamps documentation, that it follows the IEEE-754 2008 semantics.

[est31]

It seems that C has added fminimum/fmaximum following IEEE754-2019 in proposal N2532.

They have kept fmin and fmax, that follow IEEE754-2008's minNum(x,y) and maxNum(x,y), untouched for compatibility reasons.

[scottmcm]

minimum/maximum is #91079, I think?

[est31]

@scottmcm yes, good point. Despite having commented in #91008, I've not made the connection to this issue. It's interesting that LLVM errors out for the intrinsics. Maybe this should be closed in favour of #91079?

[RalfJung]

We do have minimum/maximum next to min/max now, so changing min/max NaN behavior is anyway completely off the table I assume. These are different operations in IEEE and we expose them as different operations.

But the discussion above talked a lot about NaNs when the issue really is about -0. The new operations are explicitly documented as treating -0 as less than +0 and I assume the implementation matches that. What remains is the old operations, where I don't think anything has changed?