From 4b035c9d819b830e3e3600c3d072a2c4cde29d73 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Wed, 5 Oct 2022 11:18:01 -0700 Subject: [PATCH] [release/6.0] Ensure Max/Min for floating-point on x86/x64 are not handled as commutative (#75772) * Ensure Max/Min for floating-point on x86/x64 are not handled as commutative (#75683) * 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 * Directly access the gtHWIntrinsicId field since the helper method doesn't exist in .NET 6 * Fixing a regression test to use the GetElement API available in .NET 6 --- src/coreclr/jit/gentree.cpp | 41 +++++++++++++++++-- src/coreclr/jit/hwintrinsic.h | 17 ++++++++ src/coreclr/jit/hwintrinsiclistxarch.h | 12 +++--- .../ImageSharp_2117/ImageSharp_2117.cs | 37 +++++++++++++++++ .../ImageSharp_2117/ImageSharp_2117.csproj | 13 ++++++ 5 files changed, 111 insertions(+), 9 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/ImageSharp_2117/ImageSharp_2117.cs create mode 100644 src/tests/JIT/Regression/JitBlue/ImageSharp_2117/ImageSharp_2117.csproj diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 3f196e5c254e1..58352e5a8eefc 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -18912,10 +18912,45 @@ bool GenTree::isCommutativeHWIntrinsic() const assert(gtOper == GT_HWINTRINSIC); #ifdef TARGET_XARCH - return HWIntrinsicInfo::IsCommutative(AsHWIntrinsic()->gtHWIntrinsicId); -#else - return false; + const GenTreeHWIntrinsic* node = AsHWIntrinsic(); + NamedIntrinsic id = node->gtHWIntrinsicId; + + 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 index 0b35ca719b6e2..7b1c2a9fbfa3b 100644 --- a/src/coreclr/jit/hwintrinsic.h +++ b/src/coreclr/jit/hwintrinsic.h @@ -141,6 +141,11 @@ enum HWIntrinsicFlag : unsigned int // all the intrinsic that have explicit memory load/store semantics should have this flag HW_Flag_NoContainment = 0x8000, + // 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 @@ -597,6 +602,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 index eb9bac12e3051..66cc7c4d21123 100644 --- a/src/coreclr/jit/hwintrinsiclistxarch.h +++ b/src/coreclr/jit/hwintrinsiclistxarch.h @@ -158,9 +158,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) @@ -271,10 +271,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) @@ -466,8 +466,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 new file mode 100644 index 0000000000000..b7c62091a0ea6 --- /dev/null +++ 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 fnan = Vector128.Create(float.NaN); + Vector128 res1 = Sse.Max(Sse.LoadVector128((float*)(&fnan)), Vector128.Zero); + Vector128 res2 = Sse.Min(Sse.LoadVector128((float*)(&fnan)), Vector128.Zero); + + if (float.IsNaN(res1.GetElement(0)) || float.IsNaN(res2.GetElement(0))) + { + return 0; + } + } + + if (Sse2.IsSupported) + { + Vector128 dnan = Vector128.Create(double.NaN); + Vector128 res3 = Sse2.Max(Sse2.LoadVector128((double*)(&dnan)), Vector128.Zero); + Vector128 res4 = Sse2.Min(Sse2.LoadVector128((double*)(&dnan)), Vector128.Zero); + + if (double.IsNaN(res3.GetElement(0)) || double.IsNaN(res4.GetElement(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 new file mode 100644 index 0000000000000..e7284d26ab2f1 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/ImageSharp_2117/ImageSharp_2117.csproj @@ -0,0 +1,13 @@ + + + Exe + True + + + None + True + + + + +