Runtime: Wrong handling of Vector.Min/Vector.Max of NaN

[msedi]

Description

While switching our Array math methods to Vector some issues arised in the behavior of Vector.Min and Vector.Max.
Anytime a float.NaN or double.NaN is contained in Vector<float> or Vector<double>.
This can be easily verified, by

Vector.Min(new Vector<float>(float.NaN), new Vector<float>(10))

which returns 10. Whereas a regular

MathF.Min(float.NaN,10)

returns NaN.

The biggest problem is in a vectorized method like this:

        /// <summary>
        /// B = Min(A)
        /// </summary>
        /// <param name="A"></param>
        [MethodImpl(MethodImplOptions.AggressiveInlining | MethodImplOptions.AggressiveOptimization)]
        public static T Min<T>(in ReadOnlySpan<T> A) where T : struct
        {
            T minValue = A[0];

            int N = A.Length;
            int i = 0;

            if (Vector.IsHardwareAccelerated)
            {
                int S = Vector<T>.Count;
                int R = N - (N % S);

                if (R > 2 * S)
                {

                    var minVec = new Vector<T>(A.Slice(i));
                    i += S;

                    for (; i < R; i += S)
                        minVec = Vector.Min(minVec, new Vector<T>(A.Slice(i)));

                    // Finally transport the result
                    minValue = VectorExtensions.MinHorizontalScalar(minVec);
                }
            }

            do
            {
                if ((uint)i >= (uint)N)
                    break;

                minValue = MathUtil.Min(minValue, A[i]);
                i++;
            } while (true);

            return minValue;
        }

Depending on the register size you get different results.

1. If the data length is smaller than Vector.Count you get the regular C# behavior.
2. If the data.length is equals to 2 times the Vector.Count you get the Vector.Min behavior.
3. If it is larger than 2 times Vector.Count you have some mixed mode

With this behavior it's currently a problem to use Vector.Min/Max.

Configuration

.NET Core 3.1 / Windows 10 / x64 / Release+Debug

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[EgorBo]

Currently Vector<float/double>.Max just emits vmaxps/pd which doesn't care about NaNs (and -0.0)
Vector is a crossplatform API on top of SIMD instructions so I wonder whether it should actually care about NaNs (additional overhead) or not 🤔

[msftbot[bot]]

Tagging subscribers to this area: @tannergooding, @pgovind, @jeffhandley
See info in area-owners.md if you want to be subscribed.

[msedi]

@EgorBo: You are of course right, there shouldn't be any overhead. I found this problem only since my unit tests flipped, because of a different handling of this. I think a general assumption is though that the behavior should be identical to scalar methods. I currently not sure if there are intrinsics that handle NaN correctly? The question is then if there should be an additional Min/Max method (or a parameter on Min/Max) that allow to handle it correctly?

@BTW: The origin of my improvment was Max/MinHorizontal. Something like that doesn't currently exist in Vector, right?

[EgorBo]

Here is System.Runtime.Intrinsics.x86 NaN and -0.0 aware version of Max:

    public static Vector128<float> Max(Vector128<float> v1, Vector128<float> v2)
    {
        return Sse.Or(
                Sse.And(
                    Sse.Max(v1, v2), 
                    Sse.Max(v2, v1)),
                Sse.CompareUnordered(v1, v2));
    }

Vector version would be something like this:

public static Vector<float> Max(Vector<float> v1, Vector<float> v2)
{
    return Vector.BitwiseOr(
        Vector.BitwiseAnd(
            Vector.Max(v1, v2),
            Vector.Max(v2, v1)),
        Vector.??CompareUnordered??)(v1, v2));
}

but I don't see any CompareUnordered (_mm_cmpunord_ps) API for Vector<T> (but I guess you can always cast Vector<T> to Vector128/256)

[msedi]

@EgorBo: Thx. That should do the trick.

[EgorBo]

@msedi Here is Vector<T> version:

public static Vector<float> Max(Vector<float> v1, Vector<float> v2)
{
    if (!Vector.IsHardwareAccelerated)
        return Vector.Max(v1, v2);

    // TODO: arm64

    if (Vector<float>.Count == 4)
    {
        Vector128<float> vv1 = v1.AsVector128();
        Vector128<float> vv2 = v2.AsVector128();
        return Sse.Or(Sse.And(Sse.Max(vv1, vv2), Sse.Max(vv2, vv1)),
            Sse.CompareUnordered(vv1, vv2)).AsVector();
    }
    else
    {
        // AVX
        Debug.Assert(Vector<float>.Count == 8);
        Vector256<float> vv1 = v1.AsVector256();
        Vector256<float> vv2 = v2.AsVector256();
        return Avx.Or(Avx.And(Avx.Max(vv1, vv2), Avx.Max(vv2, vv1)),
            Avx.CompareUnordered(vv1, vv2)).AsVector();
    }
}

[msedi]

@EgorBo: Thanks a lot for your help. This is really awesome. I only have one question. You are using Vector<float>.Count == 8 instead of the Sse2.IsSupported, etc. Is there any reason why you prefer this over the other?

[EgorBo]

Vector.Count == 8

ideally to check both, e.g. Vector<float>.Count == 8 && Avx.IsSupported just in case