From f3bc6de2b03049767a8f05bb88940b4b723d8ddb Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 3 Sep 2026 08:06:50 -0700 Subject: [PATCH 1/3] Fix SIMD MinMax constant special cases Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/gentree.cpp | 88 +++++++---- src/coreclr/jit/gentree.h | 6 + src/coreclr/jit/simd.h | 136 +++++++++++++++-- .../JitBlue/Runtime_133022/Runtime_133022.cs | 138 ++++++++++++++++++ .../JIT/Regression/Regression_ro_2.csproj | 1 + 5 files changed, 327 insertions(+), 42 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_133022/Runtime_133022.cs diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 269dd14cb6ab72..9f8427e06e3b32 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -20943,23 +20943,32 @@ bool GenTreeVecCon::IsBroadcast(var_types simdBaseType) const bool GenTreeVecCon::IsNaN(var_types simdBaseType) const { assert(varTypeIsFloating(simdBaseType)); - uint32_t elementCount = ElementCount(genTypeSize(gtType), simdBaseType); - for (uint32_t i = 0; i < elementCount; i++) - { - double element = GetElementFloating(simdBaseType, i); + unsigned simdSize = genTypeSize(gtType); + simd_t result = EvaluateSimdIsNaN(simdBaseType, gtSimdVal, simdSize); + return EvaluateSimdAllWhereAllBitsSet(simdBaseType, result, simdSize); +} - if (!FloatingPointUtils::isNaN(element)) - { - return false; - } - } +//------------------------------------------------------------------------ +// GenTreeVecCon::ContainsNaN: Determines if this vector constant contains a NaN +// +// Arguments: +// simdBaseType - the base type of the constant being checked +// +// Returns: +// true if any element is NaN; otherwise, false +// +bool GenTreeVecCon::ContainsNaN(var_types simdBaseType) const +{ + assert(varTypeIsFloating(simdBaseType)); - return true; + unsigned simdSize = genTypeSize(gtType); + simd_t result = EvaluateSimdIsNaN(simdBaseType, gtSimdVal, simdSize); + return EvaluateSimdAnyWhereAllBitsSet(simdBaseType, result, simdSize); } //------------------------------------------------------------------------ -// GenTreeVecCon::IsNaN: Determines if this vector constant has all elements being -0 +// GenTreeVecCon::IsNegativeZero: Determines if this vector constant has all elements being -0 // // Arguments: // simdBaseType - the base type of the constant being checked @@ -20970,19 +20979,46 @@ bool GenTreeVecCon::IsNaN(var_types simdBaseType) const bool GenTreeVecCon::IsNegativeZero(var_types simdBaseType) const { assert(varTypeIsFloating(simdBaseType)); - uint32_t elementCount = ElementCount(genTypeSize(gtType), simdBaseType); - for (uint32_t i = 0; i < elementCount; i++) - { - double element = GetElementFloating(simdBaseType, i); + unsigned simdSize = genTypeSize(gtType); + simd_t result = EvaluateSimdIsNegativeZero(simdBaseType, gtSimdVal, simdSize); + return EvaluateSimdAllWhereAllBitsSet(simdBaseType, result, simdSize); +} - if (!FloatingPointUtils::isNegativeZero(element)) - { - return false; - } - } +//------------------------------------------------------------------------ +// GenTreeVecCon::ContainsNegativeZero: Determines if this vector constant contains -0 +// +// Arguments: +// simdBaseType - the base type of the constant being checked +// +// Returns: +// true if any element is -0; otherwise, false +// +bool GenTreeVecCon::ContainsNegativeZero(var_types simdBaseType) const +{ + assert(varTypeIsFloating(simdBaseType)); - return true; + unsigned simdSize = genTypeSize(gtType); + simd_t result = EvaluateSimdIsNegativeZero(simdBaseType, gtSimdVal, simdSize); + return EvaluateSimdAnyWhereAllBitsSet(simdBaseType, result, simdSize); +} + +//------------------------------------------------------------------------ +// GenTreeVecCon::ContainsPositiveZero: Determines if this vector constant contains +0 +// +// Arguments: +// simdBaseType - the base type of the constant being checked +// +// Returns: +// true if any element is +0; otherwise, false +// +bool GenTreeVecCon::ContainsPositiveZero(var_types simdBaseType) const +{ + assert(varTypeIsFloating(simdBaseType)); + + unsigned simdSize = genTypeSize(gtType); + simd_t result = EvaluateSimdIsPositiveZero(simdBaseType, gtSimdVal, simdSize); + return EvaluateSimdAnyWhereAllBitsSet(simdBaseType, result, simdSize); } #if defined(FEATURE_MASKED_HW_INTRINSICS) @@ -26502,7 +26538,7 @@ GenTree* Compiler::gtNewSimdMinMaxNode(var_types type, if (!isMagnitude) { - bool needsFixup = false; + bool needsFixup = !isScalar && cnsNode->AsVecCon()->ContainsNaN(simdBaseType); bool canHandle = false; if (isMax) @@ -26531,7 +26567,7 @@ GenTree* Compiler::gtNewSimdMinMaxNode(var_types type, } else { - needsFixup = cnsNode->IsVectorNegativeZero(simdBaseType); + needsFixup |= cnsNode->AsVecCon()->ContainsNegativeZero(simdBaseType); } } else if (isScalar) @@ -26540,7 +26576,7 @@ GenTree* Compiler::gtNewSimdMinMaxNode(var_types type, } else { - needsFixup = cnsNode->IsVectorZero(); + needsFixup |= cnsNode->AsVecCon()->ContainsPositiveZero(simdBaseType); } if (!needsFixup || compOpportunisticallyDependsOn(InstructionSet_AVX512)) @@ -26581,7 +26617,7 @@ GenTree* Compiler::gtNewSimdMinMaxNode(var_types type, } else { - needsFixup = cnsNode->IsVectorZero(); + needsFixup |= cnsNode->AsVecCon()->ContainsPositiveZero(simdBaseType); } } else if (isScalar) @@ -26590,7 +26626,7 @@ GenTree* Compiler::gtNewSimdMinMaxNode(var_types type, } else { - needsFixup = cnsNode->IsVectorNegativeZero(simdBaseType); + needsFixup |= cnsNode->AsVecCon()->ContainsNegativeZero(simdBaseType); } if (!needsFixup || compOpportunisticallyDependsOn(InstructionSet_AVX512)) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 3da36acc01f3b0..60477341da53e0 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -7427,6 +7427,12 @@ struct GenTreeVecCon : public GenTree bool IsNegativeZero(var_types simdBaseType) const; + bool ContainsNaN(var_types simdBaseType) const; + + bool ContainsNegativeZero(var_types simdBaseType) const; + + bool ContainsPositiveZero(var_types simdBaseType) const; + bool IsZero() const { switch (gtType) diff --git a/src/coreclr/jit/simd.h b/src/coreclr/jit/simd.h index 463125b2a8aafe..a57fadc86172aa 100644 --- a/src/coreclr/jit/simd.h +++ b/src/coreclr/jit/simd.h @@ -1323,9 +1323,17 @@ inline void EvaluateBinaryMask(genTreeOps oper, #endif // FEATURE_MASKED_HW_INTRINSICS template -void EvaluateBinarySimd(genTreeOps oper, bool scalar, TSimd* result, const TSimd& arg0, const TSimd& arg1) +void EvaluateBinarySimd(genTreeOps oper, + bool scalar, + TSimd* result, + const TSimd& arg0, + const TSimd& arg1, + unsigned simdSize = sizeof(TSimd)) { - uint32_t count = sizeof(TSimd) / sizeof(TBase); + assert(simdSize <= sizeof(TSimd)); + assert((simdSize % sizeof(TBase)) == 0); + + uint32_t count = simdSize / sizeof(TBase); if (scalar) { @@ -1356,8 +1364,13 @@ void EvaluateBinarySimd(genTreeOps oper, bool scalar, TSimd* result, const TSimd } template -void EvaluateBinarySimd( - genTreeOps oper, bool scalar, var_types baseType, TSimd* result, const TSimd& arg0, const TSimd& arg1) +void EvaluateBinarySimd(genTreeOps oper, + bool scalar, + var_types baseType, + TSimd* result, + const TSimd& arg0, + const TSimd& arg1, + unsigned simdSize = sizeof(TSimd)) { switch (baseType) { @@ -1370,11 +1383,11 @@ void EvaluateBinarySimd( if (IsBinaryBitwiseOperation(oper)) { - EvaluateBinarySimd(oper, scalar, result, arg0, arg1); + EvaluateBinarySimd(oper, scalar, result, arg0, arg1, simdSize); } else { - EvaluateBinarySimd(oper, scalar, result, arg0, arg1); + EvaluateBinarySimd(oper, scalar, result, arg0, arg1, simdSize); } break; } @@ -1388,60 +1401,60 @@ void EvaluateBinarySimd( if (IsBinaryBitwiseOperation(oper)) { - EvaluateBinarySimd(oper, scalar, result, arg0, arg1); + EvaluateBinarySimd(oper, scalar, result, arg0, arg1, simdSize); } else { - EvaluateBinarySimd(oper, scalar, result, arg0, arg1); + EvaluateBinarySimd(oper, scalar, result, arg0, arg1, simdSize); } break; } case TYP_BYTE: { - EvaluateBinarySimd(oper, scalar, result, arg0, arg1); + EvaluateBinarySimd(oper, scalar, result, arg0, arg1, simdSize); break; } case TYP_SHORT: { - EvaluateBinarySimd(oper, scalar, result, arg0, arg1); + EvaluateBinarySimd(oper, scalar, result, arg0, arg1, simdSize); break; } case TYP_INT: { - EvaluateBinarySimd(oper, scalar, result, arg0, arg1); + EvaluateBinarySimd(oper, scalar, result, arg0, arg1, simdSize); break; } case TYP_LONG: { - EvaluateBinarySimd(oper, scalar, result, arg0, arg1); + EvaluateBinarySimd(oper, scalar, result, arg0, arg1, simdSize); break; } case TYP_UBYTE: { - EvaluateBinarySimd(oper, scalar, result, arg0, arg1); + EvaluateBinarySimd(oper, scalar, result, arg0, arg1, simdSize); break; } case TYP_USHORT: { - EvaluateBinarySimd(oper, scalar, result, arg0, arg1); + EvaluateBinarySimd(oper, scalar, result, arg0, arg1, simdSize); break; } case TYP_UINT: { - EvaluateBinarySimd(oper, scalar, result, arg0, arg1); + EvaluateBinarySimd(oper, scalar, result, arg0, arg1, simdSize); break; } case TYP_ULONG: { - EvaluateBinarySimd(oper, scalar, result, arg0, arg1); + EvaluateBinarySimd(oper, scalar, result, arg0, arg1, simdSize); break; } @@ -1452,6 +1465,97 @@ void EvaluateBinarySimd( } } +template +TSimd EvaluateSimdIsNaN(var_types baseType, const TSimd& value, unsigned simdSize = sizeof(TSimd)) +{ + assert(varTypeIsArithmetic(baseType)); + + TSimd result = {}; + + if (varTypeIsFloating(baseType)) + { + EvaluateBinarySimd(GT_NE, false, baseType, &result, value, value, simdSize); + } + return result; +} + +inline var_types GetSimdIntegralBaseType(var_types baseType) +{ + if (baseType == TYP_FLOAT) + { + return TYP_INT; + } + if (baseType == TYP_DOUBLE) + { + return TYP_LONG; + } + return baseType; +} + +template +TSimd EvaluateSimdIsNegative(var_types baseType, const TSimd& value, unsigned simdSize = sizeof(TSimd)) +{ + assert(varTypeIsArithmetic(baseType)); + + TSimd result = {}; + + if (!varTypeIsUnsigned(baseType)) + { + EvaluateBinarySimd(GT_LT, false, GetSimdIntegralBaseType(baseType), &result, value, TSimd::Zero(), + simdSize); + } + return result; +} + +template +TSimd EvaluateSimdIsZero(var_types baseType, const TSimd& value, unsigned simdSize = sizeof(TSimd)) +{ + TSimd result = {}; + EvaluateBinarySimd(GT_EQ, false, baseType, &result, value, TSimd::Zero(), simdSize); + return result; +} + +template +TSimd EvaluateSimdIsNegativeZero(var_types baseType, const TSimd& value, unsigned simdSize = sizeof(TSimd)) +{ + assert(varTypeIsFloating(baseType)); + + TSimd result = EvaluateSimdIsZero(baseType, value, simdSize); + TSimd sign = EvaluateSimdIsNegative(baseType, value, simdSize); + EvaluateBinarySimd(GT_AND, false, baseType, &result, result, sign, simdSize); + return result; +} + +template +TSimd EvaluateSimdIsPositiveZero(var_types baseType, const TSimd& value, unsigned simdSize = sizeof(TSimd)) +{ + assert(varTypeIsFloating(baseType)); + + TSimd result = EvaluateSimdIsZero(baseType, value, simdSize); + TSimd sign = EvaluateSimdIsNegative(baseType, value, simdSize); + EvaluateBinarySimd(GT_AND_NOT, false, baseType, &result, result, sign, simdSize); + return result; +} + +template +bool EvaluateSimdAnyWhereAllBitsSet(var_types baseType, const TSimd& value, unsigned simdSize = sizeof(TSimd)) +{ + TSimd result = {}; + TSimd zero = TSimd::Zero(); + TSimd allBitsSet = TSimd::AllBitsSet(); + EvaluateBinarySimd(GT_EQ, false, GetSimdIntegralBaseType(baseType), &result, value, allBitsSet, simdSize); + return memcmp(&result, &zero, simdSize) != 0; +} + +template +bool EvaluateSimdAllWhereAllBitsSet(var_types baseType, const TSimd& value, unsigned simdSize = sizeof(TSimd)) +{ + TSimd result = {}; + TSimd allBitsSet = TSimd::AllBitsSet(); + EvaluateBinarySimd(GT_EQ, false, GetSimdIntegralBaseType(baseType), &result, value, allBitsSet, simdSize); + return memcmp(&result, &allBitsSet, simdSize) == 0; +} + template double EvaluateGetElementFloating(var_types simdBaseType, const TSimd& arg0, int32_t arg1) { diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_133022/Runtime_133022.cs b/src/tests/JIT/Regression/JitBlue/Runtime_133022/Runtime_133022.cs new file mode 100644 index 00000000000000..ccfb1805dfb062 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_133022/Runtime_133022.cs @@ -0,0 +1,138 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Runtime_133022; + +using System; +using System.Runtime.CompilerServices; +using System.Runtime.Intrinsics; +using Xunit; + +public static class Runtime_133022 +{ + [Fact] + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + public static void TestEntryPoint() + { + Vector128 nan128 = Vector128.Create(double.NaN, double.PositiveInfinity); + Vector128 otherNan128 = Opaque(Vector128.Create(double.PositiveInfinity, double.NaN)); + Vector128 propagatedNaN128 = Vector128.Create(double.NaN); + Vector128 numberNaN128 = Vector128.Create(double.PositiveInfinity); + + AssertEqual(propagatedNaN128, Vector128.Min(nan128, otherNan128)); + AssertEqual(propagatedNaN128, Vector128.Min(otherNan128, nan128)); + AssertEqual(propagatedNaN128, Vector128.Max(nan128, otherNan128)); + AssertEqual(propagatedNaN128, Vector128.Max(otherNan128, nan128)); + AssertEqual(numberNaN128, Vector128.MinNumber(nan128, otherNan128)); + AssertEqual(numberNaN128, Vector128.MinNumber(otherNan128, nan128)); + AssertEqual(numberNaN128, Vector128.MaxNumber(nan128, otherNan128)); + AssertEqual(numberNaN128, Vector128.MaxNumber(otherNan128, nan128)); + + Vector128 zero128 = Vector128.Create(-1.0, -0.0); + Vector128 otherZero128 = Opaque(Vector128.Create(-0.0, +0.0)); + Vector128 minZero128 = Vector128.Create(-1.0, -0.0); + Vector128 maxZero128 = Vector128.Create(-0.0, +0.0); + + AssertEqual(minZero128, Vector128.Min(zero128, otherZero128)); + AssertEqual(minZero128, Vector128.Min(otherZero128, zero128)); + AssertEqual(maxZero128, Vector128.Max(zero128, otherZero128)); + AssertEqual(maxZero128, Vector128.Max(otherZero128, zero128)); + AssertEqual(minZero128, Vector128.MinNumber(zero128, otherZero128)); + AssertEqual(minZero128, Vector128.MinNumber(otherZero128, zero128)); + AssertEqual(maxZero128, Vector128.MaxNumber(zero128, otherZero128)); + AssertEqual(maxZero128, Vector128.MaxNumber(otherZero128, zero128)); + + Vector128 nanSingle128 = Vector128.Create(float.NaN, float.PositiveInfinity, 1.0f, 2.0f); + Vector128 otherNanSingle128 = Opaque(Vector128.Create(float.PositiveInfinity, float.NaN, 2.0f, 1.0f)); + + AssertEqual(Vector128.Create(float.NaN, float.NaN, 1.0f, 1.0f), Vector128.Min(nanSingle128, otherNanSingle128)); + AssertEqual(Vector128.Create(float.NaN, float.NaN, 2.0f, 2.0f), Vector128.Max(nanSingle128, otherNanSingle128)); + AssertEqual(Vector128.Create(float.PositiveInfinity, float.PositiveInfinity, 1.0f, 1.0f), + Vector128.MinNumber(nanSingle128, otherNanSingle128)); + AssertEqual(Vector128.Create(float.PositiveInfinity, float.PositiveInfinity, 2.0f, 2.0f), + Vector128.MaxNumber(nanSingle128, otherNanSingle128)); + + Vector128 zeroSingle128 = Vector128.Create(-1.0f, -0.0f, -2.0f, +0.0f); + Vector128 otherZeroSingle128 = Opaque(Vector128.Create(-0.0f, +0.0f, -3.0f, -0.0f)); + + AssertEqual(Vector128.Create(-1.0f, -0.0f, -3.0f, -0.0f), Vector128.Min(zeroSingle128, otherZeroSingle128)); + AssertEqual(Vector128.Create(-0.0f, +0.0f, -2.0f, +0.0f), Vector128.Max(zeroSingle128, otherZeroSingle128)); + AssertEqual(Vector128.Create(-1.0f, -0.0f, -3.0f, -0.0f), + Vector128.MinNumber(zeroSingle128, otherZeroSingle128)); + AssertEqual(Vector128.Create(-0.0f, +0.0f, -2.0f, +0.0f), + Vector128.MaxNumber(zeroSingle128, otherZeroSingle128)); + + Vector256 nan256 = Vector256.Create(double.NaN, double.PositiveInfinity, 1.0, 2.0); + Vector256 otherNan256 = Opaque(Vector256.Create(double.PositiveInfinity, double.NaN, 2.0, 1.0)); + Vector256 propagatedNaN256 = Vector256.Create(double.NaN, double.NaN, 1.0, 1.0); + Vector256 minNumberNaN256 = Vector256.Create(double.PositiveInfinity, double.PositiveInfinity, 1.0, 1.0); + Vector256 maxNumberNaN256 = Vector256.Create(double.PositiveInfinity, double.PositiveInfinity, 2.0, 2.0); + + AssertEqual(propagatedNaN256, Vector256.Min(nan256, otherNan256)); + AssertEqual(propagatedNaN256, Vector256.Min(otherNan256, nan256)); + AssertEqual(Vector256.Create(double.NaN, double.NaN, 2.0, 2.0), Vector256.Max(nan256, otherNan256)); + AssertEqual(Vector256.Create(double.NaN, double.NaN, 2.0, 2.0), Vector256.Max(otherNan256, nan256)); + AssertEqual(minNumberNaN256, Vector256.MinNumber(nan256, otherNan256)); + AssertEqual(minNumberNaN256, Vector256.MinNumber(otherNan256, nan256)); + AssertEqual(maxNumberNaN256, Vector256.MaxNumber(nan256, otherNan256)); + AssertEqual(maxNumberNaN256, Vector256.MaxNumber(otherNan256, nan256)); + + Vector256 zero256 = Vector256.Create(-1.0, -0.0, -2.0, +0.0); + Vector256 otherZero256 = Opaque(Vector256.Create(-0.0, +0.0, -3.0, -0.0)); + Vector256 minZero256 = Vector256.Create(-1.0, -0.0, -3.0, -0.0); + Vector256 maxZero256 = Vector256.Create(-0.0, +0.0, -2.0, +0.0); + + AssertEqual(minZero256, Vector256.Min(zero256, otherZero256)); + AssertEqual(minZero256, Vector256.Min(otherZero256, zero256)); + AssertEqual(maxZero256, Vector256.Max(zero256, otherZero256)); + AssertEqual(maxZero256, Vector256.Max(otherZero256, zero256)); + AssertEqual(minZero256, Vector256.MinNumber(zero256, otherZero256)); + AssertEqual(minZero256, Vector256.MinNumber(otherZero256, zero256)); + AssertEqual(maxZero256, Vector256.MaxNumber(zero256, otherZero256)); + AssertEqual(maxZero256, Vector256.MaxNumber(otherZero256, zero256)); + + AssertEqual(Vector128.Create(double.NaN), MinConstants128()); + AssertEqual(Vector128.Create(double.NaN), MaxConstants128()); + AssertEqual(Vector128.Create(1.0), MinWithoutSpecialValues(Opaque(Vector128.Create(3.0, 1.0)))); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static Vector128 Opaque(Vector128 value) => value; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static Vector128 Opaque(Vector128 value) => value; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static Vector256 Opaque(Vector256 value) => value; + + private static void AssertEqual(Vector128 expected, Vector128 actual) + => Assert.Equal(expected.AsUInt64(), actual.AsUInt64()); + + private static void AssertEqual(Vector128 expected, Vector128 actual) + => Assert.Equal(expected.AsUInt32(), actual.AsUInt32()); + + private static void AssertEqual(Vector256 expected, Vector256 actual) + => Assert.Equal(expected.AsUInt64(), actual.AsUInt64()); + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static Vector128 MinConstants128() + { + Vector128 left = Vector128.Create(double.NaN, 3.0); + Vector128 right = Vector128.Create(3.0, double.NaN); + + return Vector128.Min(left, right); + } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static Vector128 MaxConstants128() + { + Vector128 left = Vector128.Create(double.NaN, 3.0); + Vector128 right = Vector128.Create(3.0, double.NaN); + + return Vector128.Max(left, right); + } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + private static Vector128 MinWithoutSpecialValues(Vector128 value) + => Vector128.Min(Vector128.Create(1.0, 2.0), value); +} diff --git a/src/tests/JIT/Regression/Regression_ro_2.csproj b/src/tests/JIT/Regression/Regression_ro_2.csproj index fc4053062f98d9..24f82122800d0c 100644 --- a/src/tests/JIT/Regression/Regression_ro_2.csproj +++ b/src/tests/JIT/Regression/Regression_ro_2.csproj @@ -136,6 +136,7 @@ + From 633bdeae2f9127d57663bff9c2f849d47c26e11c Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 3 Sep 2026 15:57:30 -0700 Subject: [PATCH 2/3] Only use the single constant min/max path when the fixup can express the result The single constant AVX-512 Fixup escape hatch was broken three ways: the table was broadcast, so a non-uniform constant had the zero sign forced in every element rather than only the ones that needed it; the isNumber operand swap overwrote the Fixup node's own operands; and the ZERO fixup token does not distinguish +0 from -0, so it cannot produce the sign-dependent answer that MinNumber/MaxNumber require. Build the table per element so mixed constants are handled, restrict the fixup to the Min/Max cases it can actually express, and let everything else fall through to the general handling. A partially NaN constant cannot use the operand ordering either, so it falls through as well. Also prefer the general IR over the AVX-512 Range/Fixup sequence when both inputs are constant, since Range/Fixup does not constant fold. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/gentree.cpp | 82 ++++++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 28 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 9f8427e06e3b32..37262e797c4e8b 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -26538,8 +26538,15 @@ GenTree* Compiler::gtNewSimdMinMaxNode(var_types type, if (!isMagnitude) { - bool needsFixup = !isScalar && cnsNode->AsVecCon()->ContainsNaN(simdBaseType); - bool canHandle = false; + // xarch min/max return op2 if both inputs are 0 of either sign or if either input + // is NaN. We can exploit that to get the IEEE 754 behavior for free by ordering + // the operands such that the constant is the one that gets returned. + + // Partially NaN constants cannot use operand ordering, while mixed zero constants + // require the per-element fixup below. + bool hasPartialNaN = !isScalar && cnsNode->AsVecCon()->ContainsNaN(simdBaseType); + bool needsFixup = false; + bool canHandle = false; if (isMax) { @@ -26548,10 +26555,10 @@ GenTree* Compiler::gtNewSimdMinMaxNode(var_types type, // not be propagated for isNumber and to be propagated otherwise. // // This means for isNumber we want to do `max other, cns` and - // can only handle cns being -0 if Avx512F is supported. This is - // because if other was NaN, we want to return the non-NaN cns. - // But if cns was -0 and other was +0 we'd want to return +0 and - // so need to be able to fixup the result. + // cannot handle cns being -0. If other was NaN, we want to return + // the non-NaN cns. But if cns was -0 and other was +0 we'd want + // to return +0, and the ZERO fixup token cannot distinguish the + // opaque operand's sign. // // For !isNumber we have the inverse and want `max cns, other` and // can only handle cns being +0 if Avx512F is supported. This is @@ -26579,7 +26586,8 @@ GenTree* Compiler::gtNewSimdMinMaxNode(var_types type, needsFixup |= cnsNode->AsVecCon()->ContainsPositiveZero(simdBaseType); } - if (!needsFixup || compOpportunisticallyDependsOn(InstructionSet_AVX512)) + if (!hasPartialNaN && + (!needsFixup || (!isNumber && compOpportunisticallyDependsOn(InstructionSet_AVX512)))) { // Given the checks, op1 can safely be the cns and op2 the other node @@ -26598,10 +26606,10 @@ GenTree* Compiler::gtNewSimdMinMaxNode(var_types type, // not be propagated for isNumber and to be propagated otherwise. // // This means for isNumber we want to do `min other, cns` and - // can only handle cns being +0 if Avx512F is supported. This is - // because if other was NaN, we want to return the non-NaN cns. - // But if cns was +0 and other was -0 we'd want to return -0 and - // so need to be able to fixup the result. + // cannot handle cns being +0. If other was NaN, we want to return + // the non-NaN cns. But if cns was +0 and other was -0 we'd want + // to return -0, and the ZERO fixup token cannot distinguish the + // opaque operand's sign. // // For !isNumber we have the inverse and want `min cns, other` and // can only handle cns being -0 if Avx512F is supported. This is @@ -26629,7 +26637,8 @@ GenTree* Compiler::gtNewSimdMinMaxNode(var_types type, needsFixup |= cnsNode->AsVecCon()->ContainsNegativeZero(simdBaseType); } - if (!needsFixup || compOpportunisticallyDependsOn(InstructionSet_AVX512)) + if (!hasPartialNaN && + (!needsFixup || (!isNumber && compOpportunisticallyDependsOn(InstructionSet_AVX512)))) { // Given the checks, op1 can safely be the cns and op2 the other node @@ -26663,18 +26672,16 @@ GenTree* Compiler::gtNewSimdMinMaxNode(var_types type, retNode->AsHWIntrinsic()->Op(2) = op2; gtUpdateNodeSideEffects(retNode); - GenTreeVecCon* tblVecCon = gtNewVconNode(type); - - // FixupScalar(left, right, table, control) computes the input type of right + // Fixup(left, right, table, control) computes the input type of right // adjusts it based on the table and then returns // - // In our case, left is going to be the result of the RangeScalar operation - // and right is going to be op1 or op2. In the case op1/op2 is QNaN or SNaN - // we want to preserve it instead. Otherwise we want to preserve the original - // result computed by RangeScalar. - // - // If both inputs are NaN, then we'll end up taking op1 by virtue of it being - // the latter fixup. + // In our case, left is the result of the min/max operation and right is the + // opaque operand. The table preserves left except where the constant is the + // problematic zero. + + GenTreeVecCon* tblVecCon = gtNewVconNode(type); + int64_t tblValue; + simd_t zeroMask = {}; if (isMax) { @@ -26687,9 +26694,13 @@ GenTree* Compiler::gtNewSimdMinMaxNode(var_types type, // -VAL: 0b0000 // +VAL: 0b0000 - const int64_t tblValue = 0x00000800; - tblVecCon->EvaluateBroadcastInPlace((simdBaseType == TYP_FLOAT) ? TYP_INT : TYP_LONG, - tblValue); + tblValue = 0x00000800; + + if (!isScalar) + { + zeroMask = + EvaluateSimdIsPositiveZero(simdBaseType, cnsNode->AsVecCon()->gtSimdVal, simdSize); + } } else { @@ -26702,9 +26713,24 @@ GenTree* Compiler::gtNewSimdMinMaxNode(var_types type, // -VAL: 0b0000 // +VAL: 0b0000 - const int64_t tblValue = 0x00000700; - tblVecCon->EvaluateBroadcastInPlace((simdBaseType == TYP_FLOAT) ? TYP_INT : TYP_LONG, - tblValue); + tblValue = 0x00000700; + + if (!isScalar) + { + zeroMask = + EvaluateSimdIsNegativeZero(simdBaseType, cnsNode->AsVecCon()->gtSimdVal, simdSize); + } + } + + var_types tblType = (simdBaseType == TYP_FLOAT) ? TYP_INT : TYP_LONG; + tblVecCon->EvaluateBroadcastInPlace(tblType, tblValue); + + if (!isScalar) + { + simd_t result = {}; + EvaluateBinarySimd(GT_AND, false, tblType, &result, tblVecCon->gtSimdVal, zeroMask, + simdSize); + tblVecCon->gtSimdVal = result; } intrinsic = isScalar ? NI_AVX512_FixupScalar : NI_AVX512_Fixup; From 8dcd25b7f8e9944cd31b2d1b2af0d52d46cbf479 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 3 Sep 2026 19:32:49 -0700 Subject: [PATCH 3/3] Cover signed NaNs in vector min/max tests Add general Min/Max semantic coverage for both NaN signs and a focused mixed-lane regression for the single-constant optimization. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../tests/System/GenericMathTestMemberData.cs | 21 +++++++++++++ .../JitBlue/Runtime_133022/Runtime_133022.cs | 31 +++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/src/libraries/Common/tests/System/GenericMathTestMemberData.cs b/src/libraries/Common/tests/System/GenericMathTestMemberData.cs index 49feea063c84d7..6161d414e92b41 100644 --- a/src/libraries/Common/tests/System/GenericMathTestMemberData.cs +++ b/src/libraries/Common/tests/System/GenericMathTestMemberData.cs @@ -42,6 +42,11 @@ internal static class GenericMathTestMemberData internal const double MaxSubnormalDouble = 2.2250738585072009E-308; internal const float MaxSubnormalSingle = 1.17549421E-38f; + private static readonly double PositiveNaNDouble = BitConverter.Int64BitsToDouble(0x7FF8_0000_0000_0001); + private static readonly double NegativeNaNDouble = BitConverter.Int64BitsToDouble(unchecked((long)0xFFF8_0000_0000_0001)); + private static readonly float PositiveNaNSingle = BitConverter.Int32BitsToSingle(0x7FC0_0001); + private static readonly float NegativeNaNSingle = BitConverter.Int32BitsToSingle(unchecked((int)0xFFC0_0001)); + public static IEnumerable ClampDouble { get @@ -1367,6 +1372,8 @@ public static IEnumerable MaxDouble yield return new object[] { double.NegativeInfinity, double.NaN, double.NaN }; yield return new object[] { double.NaN, double.PositiveInfinity, double.NaN }; yield return new object[] { double.NaN, double.NegativeInfinity, double.NaN }; + yield return new object[] { PositiveNaNDouble, -0.0, PositiveNaNDouble }; + yield return new object[] { -0.0, NegativeNaNDouble, NegativeNaNDouble }; yield return new object[] { -0.0f, 0.0f, 0.0f }; yield return new object[] { 0.0f, -0.0f, 0.0f }; yield return new object[] { 2.0f, -3.0f, 2.0f }; @@ -1391,6 +1398,8 @@ public static IEnumerable MaxSingle yield return new object[] { float.NegativeInfinity, float.NaN, float.NaN }; yield return new object[] { float.NaN, float.PositiveInfinity, float.NaN }; yield return new object[] { float.NaN, float.NegativeInfinity, float.NaN }; + yield return new object[] { PositiveNaNSingle, -0.0f, PositiveNaNSingle }; + yield return new object[] { -0.0f, NegativeNaNSingle, NegativeNaNSingle }; yield return new object[] { -0.0f, 0.0f, 0.0f }; yield return new object[] { 0.0f, -0.0f, 0.0f }; yield return new object[] { 2.0f, -3.0f, 2.0f }; @@ -1511,6 +1520,8 @@ public static IEnumerable MaxNumberDouble yield return new object[] { double.NegativeInfinity, double.NaN, double.NegativeInfinity }; yield return new object[] { double.NaN, double.PositiveInfinity, double.PositiveInfinity }; yield return new object[] { double.NaN, double.NegativeInfinity, double.NegativeInfinity }; + yield return new object[] { PositiveNaNDouble, -0.0, -0.0 }; + yield return new object[] { -0.0, NegativeNaNDouble, -0.0 }; yield return new object[] { -0.0f, 0.0f, 0.0f }; yield return new object[] { 0.0f, -0.0f, 0.0f }; yield return new object[] { 2.0f, -3.0f, 2.0f }; @@ -1535,6 +1546,8 @@ public static IEnumerable MaxNumberSingle yield return new object[] { float.NegativeInfinity, float.NaN, float.NegativeInfinity }; yield return new object[] { float.NaN, float.PositiveInfinity, float.PositiveInfinity }; yield return new object[] { float.NaN, float.NegativeInfinity, float.NegativeInfinity }; + yield return new object[] { PositiveNaNSingle, -0.0f, -0.0f }; + yield return new object[] { -0.0f, NegativeNaNSingle, -0.0f }; yield return new object[] { -0.0f, 0.0f, 0.0f }; yield return new object[] { 0.0f, -0.0f, 0.0f }; yield return new object[] { 2.0f, -3.0f, 2.0f }; @@ -1559,6 +1572,8 @@ public static IEnumerable MinDouble yield return new object[] { double.NegativeInfinity, double.NaN, double.NaN }; yield return new object[] { double.NaN, double.PositiveInfinity, double.NaN }; yield return new object[] { double.NaN, double.NegativeInfinity, double.NaN }; + yield return new object[] { PositiveNaNDouble, -0.0, PositiveNaNDouble }; + yield return new object[] { -0.0, NegativeNaNDouble, NegativeNaNDouble }; yield return new object[] { -0.0f, 0.0f, -0.0f }; yield return new object[] { 0.0f, -0.0f, -0.0f }; yield return new object[] { 2.0f, -3.0f, -3.0f }; @@ -1583,6 +1598,8 @@ public static IEnumerable MinSingle yield return new object[] { float.NegativeInfinity, float.NaN, float.NaN }; yield return new object[] { float.NaN, float.PositiveInfinity, float.NaN }; yield return new object[] { float.NaN, float.NegativeInfinity, float.NaN }; + yield return new object[] { PositiveNaNSingle, -0.0f, PositiveNaNSingle }; + yield return new object[] { -0.0f, NegativeNaNSingle, NegativeNaNSingle }; yield return new object[] { -0.0f, 0.0f, -0.0f }; yield return new object[] { 0.0f, -0.0f, -0.0f }; yield return new object[] { 2.0f, -3.0f, -3.0f }; @@ -1703,6 +1720,8 @@ public static IEnumerable MinNumberDouble yield return new object[] { double.NegativeInfinity, double.NaN, double.NegativeInfinity }; yield return new object[] { double.NaN, double.PositiveInfinity, double.PositiveInfinity }; yield return new object[] { double.NaN, double.NegativeInfinity, double.NegativeInfinity }; + yield return new object[] { PositiveNaNDouble, -0.0, -0.0 }; + yield return new object[] { -0.0, NegativeNaNDouble, -0.0 }; yield return new object[] { -0.0f, 0.0f, -0.0f }; yield return new object[] { 0.0f, -0.0f, -0.0f }; yield return new object[] { 2.0f, -3.0f, -3.0f }; @@ -1727,6 +1746,8 @@ public static IEnumerable MinNumberSingle yield return new object[] { float.NegativeInfinity, float.NaN, float.NegativeInfinity }; yield return new object[] { float.NaN, float.PositiveInfinity, float.PositiveInfinity }; yield return new object[] { float.NaN, float.NegativeInfinity, float.NegativeInfinity }; + yield return new object[] { PositiveNaNSingle, -0.0f, -0.0f }; + yield return new object[] { -0.0f, NegativeNaNSingle, -0.0f }; yield return new object[] { -0.0f, 0.0f, -0.0f }; yield return new object[] { 0.0f, -0.0f, -0.0f }; yield return new object[] { 2.0f, -3.0f, -3.0f }; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_133022/Runtime_133022.cs b/src/tests/JIT/Regression/JitBlue/Runtime_133022/Runtime_133022.cs index ccfb1805dfb062..35a71919922311 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_133022/Runtime_133022.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_133022/Runtime_133022.cs @@ -62,6 +62,18 @@ public static void TestEntryPoint() AssertEqual(Vector128.Create(-0.0f, +0.0f, -2.0f, +0.0f), Vector128.MaxNumber(zeroSingle128, otherZeroSingle128)); + float positiveNaN = BitConverter.Int32BitsToSingle(0x7FC0_0001); + float negativeNaN = BitConverter.Int32BitsToSingle(unchecked((int)0xFFC0_0001)); + Vector128 mixedSingle128 = Vector128.Create(-1.0f, -2.0f, -0.0f, +0.0f); + Vector128 otherMixedSingle128 = Opaque(Vector128.Create(positiveNaN, negativeNaN, +0.0f, -0.0f)); + Vector128 minMixedSingle128 = Vector128.Create(float.NaN, float.NaN, -0.0f, -0.0f); + Vector128 maxMixedSingle128 = Vector128.Create(float.NaN, float.NaN, +0.0f, +0.0f); + + AssertEqualIgnoringNaNBits(minMixedSingle128, Vector128.Min(mixedSingle128, otherMixedSingle128)); + AssertEqualIgnoringNaNBits(minMixedSingle128, Vector128.Min(otherMixedSingle128, mixedSingle128)); + AssertEqualIgnoringNaNBits(maxMixedSingle128, Vector128.Max(mixedSingle128, otherMixedSingle128)); + AssertEqualIgnoringNaNBits(maxMixedSingle128, Vector128.Max(otherMixedSingle128, mixedSingle128)); + Vector256 nan256 = Vector256.Create(double.NaN, double.PositiveInfinity, 1.0, 2.0); Vector256 otherNan256 = Opaque(Vector256.Create(double.PositiveInfinity, double.NaN, 2.0, 1.0)); Vector256 propagatedNaN256 = Vector256.Create(double.NaN, double.NaN, 1.0, 1.0); @@ -111,6 +123,25 @@ private static void AssertEqual(Vector128 expected, Vector128 ac private static void AssertEqual(Vector128 expected, Vector128 actual) => Assert.Equal(expected.AsUInt32(), actual.AsUInt32()); + private static void AssertEqualIgnoringNaNBits(Vector128 expected, Vector128 actual) + { + for (int index = 0; index < Vector128.Count; index++) + { + float expectedElement = expected.GetElement(index); + float actualElement = actual.GetElement(index); + + if (float.IsNaN(expectedElement)) + { + Assert.True(float.IsNaN(actualElement)); + } + else + { + Assert.Equal(BitConverter.SingleToInt32Bits(expectedElement), + BitConverter.SingleToInt32Bits(actualElement)); + } + } + } + private static void AssertEqual(Vector256 expected, Vector256 actual) => Assert.Equal(expected.AsUInt64(), actual.AsUInt64());