[release/7.0-rc2] Ensure Max/Min for floating-point on x86/x64 are no…

…t handled as commutative (#75761) * Adding a regression test for SixLabors/ImageSharp#2117 * Ensure Max/Min for floating-point on x86/x64 are not handled as commutative * Applying formatting patch Co-authored-by: Tanner Gooding <[email protected]>
dotnet · ViktorHofer · Sep 20, 2022 · Sep 15, 2022 · Sep 15, 2022 · Sep 16, 2022
commit 5634242a3a8bd4d2f35e063537d7350939162e80
diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp
@@ -18631,10 +18631,45 @@ bool GenTree::isCommutativeHWIntrinsic() const
     assert(gtOper == GT_HWINTRINSIC);
 
 #ifdef TARGET_XARCH
-    return HWIntrinsicInfo::IsCommutative(AsHWIntrinsic()->GetHWIntrinsicId());
-#else
-    return false;
+    const GenTreeHWIntrinsic* node = AsHWIntrinsic();
+    NamedIntrinsic            id   = node->GetHWIntrinsicId();
+
+    if (HWIntrinsicInfo::IsCommutative(id))
+    {
+        return true;
+    }
+
+    if (HWIntrinsicInfo::IsMaybeCommutative(id))
+    {
+        switch (id)
+        {
+            case NI_SSE_Max:
+            case NI_SSE_Min:
+            {
+                return false;
+            }
+
+            case NI_SSE2_Max:
+            case NI_SSE2_Min:
+            {
+                return !varTypeIsFloating(node->GetSimdBaseType());
+            }
+
+            case NI_AVX_Max:
+            case NI_AVX_Min:
+            {
+                return false;
+            }
+
+            default:
+            {
+                unreached();
+            }
+        }
+    }
 #endif // TARGET_XARCH
+
+    return false;
 }
 
 bool GenTree::isContainableHWIntrinsic() const

diff --git a/src/coreclr/jit/hwintrinsic.h b/src/coreclr/jit/hwintrinsic.h
@@ -153,6 +153,11 @@ enum HWIntrinsicFlag : unsigned int
     // the intrinsic can be used on hardware with AVX but not AVX2 support
     HW_Flag_AvxOnlyCompatible = 0x40000,
 
+    // MaybeCommutative
+    // - if a binary-op intrinsic is maybe commutative (e.g., Max or Min for float/double), its op1 can possibly be
+    // contained
+    HW_Flag_MaybeCommutative = 0x80000,
+
 #elif defined(TARGET_ARM64)
     // The intrinsic has an immediate operand
     // - the value can be (and should be) encoded in a corresponding instruction when the operand value is constant
@@ -626,6 +631,18 @@ struct HWIntrinsicInfo
         return (flags & HW_Flag_Commutative) != 0;
     }
 
+    static bool IsMaybeCommutative(NamedIntrinsic id)
+    {
+        HWIntrinsicFlag flags = lookupFlags(id);
+#if defined(TARGET_XARCH)
+        return (flags & HW_Flag_MaybeCommutative) != 0;
+#elif defined(TARGET_ARM64)
+        return false;
+#else
+#error Unsupported platform
+#endif
+    }
+
     static bool RequiresCodegen(NamedIntrinsic id)
     {
         HWIntrinsicFlag flags = lookupFlags(id);

diff --git a/src/coreclr/jit/hwintrinsiclistxarch.h b/src/coreclr/jit/hwintrinsiclistxarch.h
@@ -290,9 +290,9 @@ HARDWARE_INTRINSIC(SSE,             LoadHigh,
 HARDWARE_INTRINSIC(SSE,             LoadLow,                                    16,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_movlps,             INS_invalid},           HW_Category_MemoryLoad,             HW_Flag_NoRMWSemantics)
 HARDWARE_INTRINSIC(SSE,             LoadScalarVector128,                        16,              1,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_movss,              INS_invalid},           HW_Category_MemoryLoad,             HW_Flag_NoRMWSemantics)
 HARDWARE_INTRINSIC(SSE,             LoadVector128,                              16,              1,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_movups,             INS_invalid},           HW_Category_MemoryLoad,             HW_Flag_NoRMWSemantics)
-HARDWARE_INTRINSIC(SSE,             Max,                                        16,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_maxps,              INS_invalid},           HW_Category_SimpleSIMD,             HW_Flag_Commutative)
+HARDWARE_INTRINSIC(SSE,             Max,                                        16,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_maxps,              INS_invalid},           HW_Category_SimpleSIMD,             HW_Flag_MaybeCommutative)
 HARDWARE_INTRINSIC(SSE,             MaxScalar,                                  16,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_maxss,              INS_invalid},           HW_Category_SIMDScalar,             HW_Flag_CopyUpperBits)
-HARDWARE_INTRINSIC(SSE,             Min,                                        16,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_minps,              INS_invalid},           HW_Category_SimpleSIMD,             HW_Flag_Commutative)
+HARDWARE_INTRINSIC(SSE,             Min,                                        16,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_minps,              INS_invalid},           HW_Category_SimpleSIMD,             HW_Flag_MaybeCommutative)
 HARDWARE_INTRINSIC(SSE,             MinScalar,                                  16,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_minss,              INS_invalid},           HW_Category_SIMDScalar,             HW_Flag_CopyUpperBits)
 HARDWARE_INTRINSIC(SSE,             MoveHighToLow,                              16,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_movhlps,            INS_invalid},           HW_Category_SimpleSIMD,             HW_Flag_NoContainment)
 HARDWARE_INTRINSIC(SSE,             MoveLowToHigh,                              16,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_movlhps,            INS_invalid},           HW_Category_SimpleSIMD,             HW_Flag_NoContainment)
@@ -403,10 +403,10 @@ HARDWARE_INTRINSIC(SSE2,            LoadLow,
 HARDWARE_INTRINSIC(SSE2,            LoadScalarVector128,                        16,              1,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_movd,               INS_movd,               INS_movq,               INS_movq,               INS_invalid,            INS_movsdsse2},         HW_Category_MemoryLoad,             HW_Flag_NoRMWSemantics)
 HARDWARE_INTRINSIC(SSE2,            LoadVector128,                              16,              1,     {INS_movdqu,            INS_movdqu,             INS_movdqu,             INS_movdqu,             INS_movdqu,             INS_movdqu,             INS_movdqu,             INS_movdqu,             INS_invalid,            INS_movupd},            HW_Category_MemoryLoad,             HW_Flag_NoRMWSemantics)
 HARDWARE_INTRINSIC(SSE2,            MaskMove,                                   16,              3,     {INS_maskmovdqu,        INS_maskmovdqu,         INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid},           HW_Category_MemoryStore,            HW_Flag_NoContainment|HW_Flag_NoRMWSemantics|HW_Flag_BaseTypeFromSecondArg)
-HARDWARE_INTRINSIC(SSE2,            Max,                                        16,              2,     {INS_invalid,           INS_pmaxub,             INS_pmaxsw,             INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_maxpd},             HW_Category_SimpleSIMD,             HW_Flag_Commutative)
+HARDWARE_INTRINSIC(SSE2,            Max,                                        16,              2,     {INS_invalid,           INS_pmaxub,             INS_pmaxsw,             INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_maxpd},             HW_Category_SimpleSIMD,             HW_Flag_MaybeCommutative)
 HARDWARE_INTRINSIC(SSE2,            MemoryFence,                                 0,              0,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid},           HW_Category_Special,                HW_Flag_NoContainment|HW_Flag_NoRMWSemantics)
 HARDWARE_INTRINSIC(SSE2,            MaxScalar,                                  16,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_maxsd},             HW_Category_SIMDScalar,             HW_Flag_CopyUpperBits)
-HARDWARE_INTRINSIC(SSE2,            Min,                                        16,              2,     {INS_invalid,           INS_pminub,             INS_pminsw,             INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_minpd},             HW_Category_SimpleSIMD,             HW_Flag_Commutative)
+HARDWARE_INTRINSIC(SSE2,            Min,                                        16,              2,     {INS_invalid,           INS_pminub,             INS_pminsw,             INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_minpd},             HW_Category_SimpleSIMD,             HW_Flag_MaybeCommutative)
 HARDWARE_INTRINSIC(SSE2,            MinScalar,                                  16,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_minsd},             HW_Category_SIMDScalar,             HW_Flag_CopyUpperBits)
 HARDWARE_INTRINSIC(SSE2,            MoveMask,                                   16,              1,     {INS_pmovmskb,          INS_pmovmskb,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_movmskpd},          HW_Category_SimpleSIMD,             HW_Flag_NoContainment|HW_Flag_NoRMWSemantics|HW_Flag_BaseTypeFromFirstArg)
 HARDWARE_INTRINSIC(SSE2,            MoveScalar,                                 16,             -1,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_movq,               INS_movq,               INS_invalid,            INS_movsdsse2},         HW_Category_SIMDScalar,             HW_Flag_NoContainment)
@@ -598,8 +598,8 @@ HARDWARE_INTRINSIC(AVX,             InsertVector128,
 HARDWARE_INTRINSIC(AVX,             LoadAlignedVector256,                       32,              1,     {INS_movdqa,            INS_movdqa,             INS_movdqa,             INS_movdqa,             INS_movdqa,             INS_movdqa,             INS_movdqa,             INS_movdqa,             INS_movaps,             INS_movapd},            HW_Category_MemoryLoad,             HW_Flag_NoRMWSemantics)
 HARDWARE_INTRINSIC(AVX,             LoadDquVector256,                           32,              1,     {INS_lddqu,             INS_lddqu,              INS_lddqu,              INS_lddqu,              INS_lddqu,              INS_lddqu,              INS_lddqu,              INS_lddqu,              INS_invalid,            INS_invalid},           HW_Category_MemoryLoad,             HW_Flag_NoRMWSemantics)
 HARDWARE_INTRINSIC(AVX,             LoadVector256,                              32,              1,     {INS_movdqu,            INS_movdqu,             INS_movdqu,             INS_movdqu,             INS_movdqu,             INS_movdqu,             INS_movdqu,             INS_movdqu,             INS_movups,             INS_movupd},            HW_Category_MemoryLoad,             HW_Flag_NoRMWSemantics)
-HARDWARE_INTRINSIC(AVX,             Max,                                        32,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_maxps,              INS_maxpd},             HW_Category_SimpleSIMD,             HW_Flag_Commutative)
-HARDWARE_INTRINSIC(AVX,             Min,                                        32,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_minps,              INS_minpd},             HW_Category_SimpleSIMD,             HW_Flag_Commutative)
+HARDWARE_INTRINSIC(AVX,             Max,                                        32,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_maxps,              INS_maxpd},             HW_Category_SimpleSIMD,             HW_Flag_MaybeCommutative)
+HARDWARE_INTRINSIC(AVX,             Min,                                        32,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_minps,              INS_minpd},             HW_Category_SimpleSIMD,             HW_Flag_MaybeCommutative)
 HARDWARE_INTRINSIC(AVX,             MaskLoad,                                   -1,              2,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_vmaskmovps,         INS_vmaskmovpd},        HW_Category_MemoryLoad,             HW_Flag_NoFlag)
 HARDWARE_INTRINSIC(AVX,             MaskStore,                                  -1,              3,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_vmaskmovps,         INS_vmaskmovpd},        HW_Category_MemoryStore,            HW_Flag_NoContainment|HW_Flag_BaseTypeFromSecondArg)
 HARDWARE_INTRINSIC(AVX,             MoveMask,                                   32,              1,     {INS_invalid,           INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_invalid,            INS_movmskps,           INS_movmskpd},          HW_Category_SimpleSIMD,             HW_Flag_NoContainment|HW_Flag_BaseTypeFromFirstArg)

diff --git a/src/tests/JIT/Regression/JitBlue/ImageSharp_2117/ImageSharp_2117.cs b/src/tests/JIT/Regression/JitBlue/ImageSharp_2117/ImageSharp_2117.cs
@@ -0,0 +1,37 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System.Runtime.Intrinsics;
+using System.Runtime.Intrinsics.X86;
+
+public unsafe class ImageSharp_2117
+{
+    public static int Main(string[] args)
+    {
+        if (Sse.IsSupported)
+        {
+            Vector128<float> fnan = Vector128.Create(float.NaN);
+            Vector128<float> res1 = Sse.Max(Sse.LoadVector128((float*)(&fnan)), Vector128<float>.Zero);
+            Vector128<float> res2 = Sse.Min(Sse.LoadVector128((float*)(&fnan)), Vector128<float>.Zero);
+
+            if (float.IsNaN(res1[0]) || float.IsNaN(res2[0]))
+            {
+                return 0;
+            }
+        }
+
+        if (Sse2.IsSupported)
+        {
+            Vector128<double> dnan = Vector128.Create(double.NaN);
+            Vector128<double> res3 = Sse2.Max(Sse2.LoadVector128((double*)(&dnan)), Vector128<double>.Zero);
+            Vector128<double> res4 = Sse2.Min(Sse2.LoadVector128((double*)(&dnan)), Vector128<double>.Zero);
+
+            if (double.IsNaN(res3[0]) || double.IsNaN(res4[0]))
+            {
+                return 0;
+            }
+        }
+
+        return 100;
+    }
+}
diff --git a/src/tests/JIT/Regression/JitBlue/ImageSharp_2117/ImageSharp_2117.csproj b/src/tests/JIT/Regression/JitBlue/ImageSharp_2117/ImageSharp_2117.csproj
@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>