Fix Ord and PartialOrd differing for FloatOrd and optimize implementation

[Kanabenki]

Objective

• FloatOrd currently has a different comparison behavior between its derived PartialOrd impl and manually implemented Ord impl (The Ord doc says this is a logic error). This might be a problem for some std containers/algorithms if they rely on both matching, and a footgun for Bevy users.

Solution

• Replace the PartialEq and Ord impls of FloatOrd with some equivalent ones producing better assembly.
• Manually derive PartialOrd with the same behavior as Ord, implement the comparison operators.
• Add some tests.

I first tried using a match-based implementation similar to the PartialOrd impl of the std (with added NaN ordering) but I couldn't get it to produce non-branching assembly. The current implementation is based on the one from the ordered_float crate, adapted since it uses a different ordering. Should this be mentionned somewhere in the code?

---

Changelog

Fixed

• FloatOrd now uses the same ordering for its PartialOrd and Ord implementations.

Migration Guide

• If you were depending on the PartialOrd behaviour of FloatOrd, it has changed from matching f32 to matching FloatOrd's Ord ordering, never returning None.

[hymm]

could we add some tests for le too, since the behavior there is a little weird and should match cmp?

[Kanabenki]

Done for all operators, also added a test for the Hash impl.

[bushrat011899]

I think this is a good idea, but I personally don't like that the core implementation of the behaviour is split between PartialOrd::le and PartialEq::eq. If instead we provide an explicit implementation for Ord::cmp, all the other methods can be derived/implied:

// Actual Implementation
impl Ord for FloatOrd {
    fn cmp(&self, other: &Self) -> Ordering {
        self.0.partial_cmp(&other.0).unwrap_or_else(|| {
            // Comparison could only fail if one of self or other is NaN.
            // All NaN values are equivalent and less than all non-NaN values
            if !other.0.is_nan() {
                Ordering::Less
            } else if !self.0.is_nan() {
                Ordering::Greater
            } else {
                Ordering::Equal
            }
        })
    }
}

// Derived from Above
impl PartialEq for FloatOrd {
    fn eq(&self, other: &Self) -> bool {
        self.cmp(other).is_eq()
    }
}

impl PartialOrd for FloatOrd {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(other))
    }
}

impl Eq for FloatOrd {}

I personally prefer this, since there is only 1 function to actually test and reason around, and it's very clear how it relates to all other comparisons.

[Kanabenki]

I think this is a good idea, but I personally don't like that the core implementation of the behaviour is split between PartialOrd::le and PartialEq::eq. If instead we provide an explicit implementation for Ord::cmp, all the other methods can be derived/implied:

I agree that this would be easier to maintain and that was the very first thing I tried, but the produced assembly is worse (notably, having cmp implemented in term of the manually implemented comparison operators instead of the other way around allows the operators to be better optimized). Since FloatOrd is used as a sorting key in rendering, I think would make sense to try to keep it as optimized as possible, even if the resulting impl is a bit less straightforward.

It seems that keeping the current cmp impl of this PR but deferring to it for Eq also produce worse assembly for eq.

[alice-i-cecile]

This will conflict with #12732: we should be careful not to lose this work in the process.

[bushrat011899]

I agree that this would be easier to maintain and that was the very first thing I tried, but the produced assembly is worse

Unfortunate, but I completely agree. The performance of this item is important enough to make the sacrifice reasonable.