From 13cc10d72882000fbf7e452abb5acb58846e5589 Mon Sep 17 00:00:00 2001 From: TIHan Date: Wed, 20 Sep 2023 19:17:22 -0700 Subject: [PATCH 1/9] Added OperIsHWIntrinsicSIMDScalar. Do not remove CAST on SIMD scalar operations for stores. --- src/coreclr/jit/gentree.cpp | 35 +++++++++++++++++++++++++++++++++++ src/coreclr/jit/gentree.h | 2 ++ src/coreclr/jit/morph.cpp | 10 ++++++++++ 3 files changed, 47 insertions(+) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index d42175f98300ef..dddda7886be83d 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -26448,3 +26448,38 @@ bool GenTree::CanDivOrModPossiblyOverflow(Compiler* comp) const // Not enough known information; therefore we might overflow. return true; } + +//------------------------------------------------------------------------ +// OperIsHWIntrinsicSIMDScalar: returns true if the given tree is GT_HWINTRINSIC and +// is a SIMD scalar operation +// +// Return Value: +// true if the given tree is a hwintrinsic and a SIMD scalar operation +// +bool GenTree::OperIsHWIntrinsicSIMDScalar() +{ + if (!this->OperIsHWIntrinsic()) + return false; + +#if defined(FEATURE_HW_INTRINSICS) + GenTreeHWIntrinsic* hwintrinsic = this->AsHWIntrinsic(); + NamedIntrinsic intrinsicId = hwintrinsic->GetHWIntrinsicId(); + +#if defined(TARGET_AMD64) + HWIntrinsicCategory category = HWIntrinsicInfo::lookupCategory(hwintrinsic->GetHWIntrinsicId()); + + switch (category) + { + case HW_Category_SIMDScalar: + return true; + + default: + break; + } +#elif defined(TARGET_ARM64) + return HWIntrinsicInfo::SIMDScalar(intrinsicId); +#endif // TARGET_ARM64 && !TARGET_AMD64 +#endif // FEATURE_HW_INTRINSICS + + return false; +} diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index b0f7536f261de5..536e73cb6ca00e 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -1653,6 +1653,8 @@ struct GenTree bool OperIsHWIntrinsic(NamedIntrinsic intrinsicId) const; + bool OperIsHWIntrinsicSIMDScalar(); + // This is here for cleaner GT_LONG #ifdefs. static bool OperIsLong(genTreeOps gtOper) { diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 39617fb20a27d8..dda33379ce8856 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -10086,6 +10086,11 @@ GenTree* Compiler::fgOptimizeCastOnStore(GenTree* store) if (!src->OperIs(GT_CAST)) return store; + // SIMD scalar operations may be specialized for stores, but not when the store expects a small type. + // Therefore, we keep the CAST. + if (src->gtGetOp1()->OperIsHWIntrinsicSIMDScalar() && varTypeIsSmall(store)) + return store; + if (store->OperIs(GT_STORE_LCL_VAR)) { LclVarDsc* varDsc = lvaGetDesc(store->AsLclVarCommon()->GetLclNum()); @@ -11894,6 +11899,11 @@ GenTree* Compiler::fgMorphSmpOpOptional(GenTreeOp* tree, bool* optAssertionPropD if (op2->gtOper == GT_CAST && !op2->gtOverflow()) { + // SIMD scalar operations may be specialized for stores, but not when the store expects a small type. + // Therefore, we keep the CAST. + if (op2->gtGetOp1()->OperIsHWIntrinsicSIMDScalar() && varTypeIsSmall(tree)) + break; + var_types srct; var_types cast; var_types dstt; From 535f3a40e4611bb8bc4eac30b0195a3d29d5e69d Mon Sep 17 00:00:00 2001 From: TIHan Date: Wed, 20 Sep 2023 19:37:11 -0700 Subject: [PATCH 2/9] Minor cleanup --- src/coreclr/jit/gentree.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index dddda7886be83d..bf2cf3145caf8b 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -26466,9 +26466,7 @@ bool GenTree::OperIsHWIntrinsicSIMDScalar() NamedIntrinsic intrinsicId = hwintrinsic->GetHWIntrinsicId(); #if defined(TARGET_AMD64) - HWIntrinsicCategory category = HWIntrinsicInfo::lookupCategory(hwintrinsic->GetHWIntrinsicId()); - - switch (category) + switch (HWIntrinsicInfo::lookupCategory(intrinsicId)) { case HW_Category_SIMDScalar: return true; From 3886fa04a3293b77576bffa9359684a9870ae655 Mon Sep 17 00:00:00 2001 From: TIHan Date: Wed, 20 Sep 2023 19:38:46 -0700 Subject: [PATCH 3/9] Minor cleanup --- src/coreclr/jit/gentree.cpp | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index bf2cf3145caf8b..c3dcd960541d4d 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -26458,25 +26458,20 @@ bool GenTree::CanDivOrModPossiblyOverflow(Compiler* comp) const // bool GenTree::OperIsHWIntrinsicSIMDScalar() { +#if defined(FEATURE_HW_INTRINSICS) + if (!this->OperIsHWIntrinsic()) return false; -#if defined(FEATURE_HW_INTRINSICS) GenTreeHWIntrinsic* hwintrinsic = this->AsHWIntrinsic(); NamedIntrinsic intrinsicId = hwintrinsic->GetHWIntrinsicId(); #if defined(TARGET_AMD64) - switch (HWIntrinsicInfo::lookupCategory(intrinsicId)) - { - case HW_Category_SIMDScalar: - return true; - - default: - break; - } + return HWIntrinsicInfo::lookupCategory(intrinsicId) == HW_Category_SIMDScalar; #elif defined(TARGET_ARM64) return HWIntrinsicInfo::SIMDScalar(intrinsicId); #endif // TARGET_ARM64 && !TARGET_AMD64 + #endif // FEATURE_HW_INTRINSICS return false; From e8fdb85958dd1f00d55f0ad4aebd6ba49f5dd1a8 Mon Sep 17 00:00:00 2001 From: TIHan Date: Thu, 21 Sep 2023 03:48:17 -0700 Subject: [PATCH 4/9] Feedback --- src/coreclr/jit/gentree.cpp | 28 ---------------------------- src/coreclr/jit/gentree.h | 2 -- src/coreclr/jit/lowerxarch.cpp | 2 +- src/coreclr/jit/morph.cpp | 10 ---------- 4 files changed, 1 insertion(+), 41 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index c3dcd960541d4d..d42175f98300ef 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -26448,31 +26448,3 @@ bool GenTree::CanDivOrModPossiblyOverflow(Compiler* comp) const // Not enough known information; therefore we might overflow. return true; } - -//------------------------------------------------------------------------ -// OperIsHWIntrinsicSIMDScalar: returns true if the given tree is GT_HWINTRINSIC and -// is a SIMD scalar operation -// -// Return Value: -// true if the given tree is a hwintrinsic and a SIMD scalar operation -// -bool GenTree::OperIsHWIntrinsicSIMDScalar() -{ -#if defined(FEATURE_HW_INTRINSICS) - - if (!this->OperIsHWIntrinsic()) - return false; - - GenTreeHWIntrinsic* hwintrinsic = this->AsHWIntrinsic(); - NamedIntrinsic intrinsicId = hwintrinsic->GetHWIntrinsicId(); - -#if defined(TARGET_AMD64) - return HWIntrinsicInfo::lookupCategory(intrinsicId) == HW_Category_SIMDScalar; -#elif defined(TARGET_ARM64) - return HWIntrinsicInfo::SIMDScalar(intrinsicId); -#endif // TARGET_ARM64 && !TARGET_AMD64 - -#endif // FEATURE_HW_INTRINSICS - - return false; -} diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 536e73cb6ca00e..b0f7536f261de5 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -1653,8 +1653,6 @@ struct GenTree bool OperIsHWIntrinsic(NamedIntrinsic intrinsicId) const; - bool OperIsHWIntrinsicSIMDScalar(); - // This is here for cleaner GT_LONG #ifdefs. static bool OperIsLong(genTreeOps gtOper) { diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index d68c3e0d2689a2..71b700ddd97ebf 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -6504,7 +6504,7 @@ void Lowering::ContainCheckStoreIndir(GenTreeStoreInd* node) case NI_AVX2_ConvertToUInt32: { // These intrinsics are "ins reg/mem, xmm" - isContainable = varTypeIsIntegral(simdBaseType); + isContainable = varTypeIsIntegral(simdBaseType) && (genTypeSize(src) == genTypeSize(node)); break; } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index dda33379ce8856..39617fb20a27d8 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -10086,11 +10086,6 @@ GenTree* Compiler::fgOptimizeCastOnStore(GenTree* store) if (!src->OperIs(GT_CAST)) return store; - // SIMD scalar operations may be specialized for stores, but not when the store expects a small type. - // Therefore, we keep the CAST. - if (src->gtGetOp1()->OperIsHWIntrinsicSIMDScalar() && varTypeIsSmall(store)) - return store; - if (store->OperIs(GT_STORE_LCL_VAR)) { LclVarDsc* varDsc = lvaGetDesc(store->AsLclVarCommon()->GetLclNum()); @@ -11899,11 +11894,6 @@ GenTree* Compiler::fgMorphSmpOpOptional(GenTreeOp* tree, bool* optAssertionPropD if (op2->gtOper == GT_CAST && !op2->gtOverflow()) { - // SIMD scalar operations may be specialized for stores, but not when the store expects a small type. - // Therefore, we keep the CAST. - if (op2->gtGetOp1()->OperIsHWIntrinsicSIMDScalar() && varTypeIsSmall(tree)) - break; - var_types srct; var_types cast; var_types dstt; From 58b466dc85dd9202061917674abaa8b0e899970d Mon Sep 17 00:00:00 2001 From: TIHan Date: Thu, 21 Sep 2023 13:03:38 -0700 Subject: [PATCH 5/9] Added test case --- .../JitBlue/Runtime_92349/Runtime_92349.cs | 24 +++++++++++++++++++ .../Runtime_92349/Runtime_92349.csproj | 9 +++++++ 2 files changed, 33 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs new file mode 100644 index 00000000000000..27a029881c4ad0 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs @@ -0,0 +1,24 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; +using System.Threading; +using Xunit; + +public static class Runtime_92349 +{ + [MethodImpl(MethodImplOptions.AggressiveOptimization)] + unsafe static void Test(byte* pValue) + { + *pValue = (byte)Sse2.ConvertToInt32(Vector128.Create(-10, 0, 0, 0)); + } + + [Fact] + unsafe static void EntryPoint() + { + ulong value = 0; + Test((byte*)Unsafe.AsPointer(ref value)); + Assert.True(value == 246); + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.csproj new file mode 100644 index 00000000000000..6bb210527e0797 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.csproj @@ -0,0 +1,9 @@ + + + True + True + + + + + \ No newline at end of file From 714ea8a8edf3fc6bb4858c5d46f759fb0a3f96d7 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Thu, 21 Sep 2023 15:18:48 -0700 Subject: [PATCH 6/9] Update Runtime_92349.cs --- .../JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs index 27a029881c4ad0..ddfe33cafffa4a 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs @@ -9,16 +9,16 @@ public static class Runtime_92349 { [MethodImpl(MethodImplOptions.AggressiveOptimization)] - unsafe static void Test(byte* pValue) + public unsafe static void Test(byte* pValue) { *pValue = (byte)Sse2.ConvertToInt32(Vector128.Create(-10, 0, 0, 0)); } [Fact] - unsafe static void EntryPoint() + public unsafe static void EntryPoint() { ulong value = 0; Test((byte*)Unsafe.AsPointer(ref value)); Assert.True(value == 246); } -} \ No newline at end of file +} From f08f4d9e680be5a857cbea03b957eeda09d3581b Mon Sep 17 00:00:00 2001 From: Will Smith Date: Fri, 22 Sep 2023 11:17:17 -0700 Subject: [PATCH 7/9] Update Runtime_92349.cs --- src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs index ddfe33cafffa4a..69a82bdc6f588f 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs @@ -2,6 +2,8 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Runtime.Intrinsics.X86; +using System.Runtime.Intrinsics; using System.Runtime.CompilerServices; using System.Threading; using Xunit; From 956eef75c483c2b7b6ccf92af58317fbb3e7628b Mon Sep 17 00:00:00 2001 From: Will Smith Date: Fri, 22 Sep 2023 12:30:57 -0700 Subject: [PATCH 8/9] Update Runtime_92349.cs --- src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs index 69a82bdc6f588f..79769714c394ab 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs @@ -11,7 +11,7 @@ public static class Runtime_92349 { [MethodImpl(MethodImplOptions.AggressiveOptimization)] - public unsafe static void Test(byte* pValue) + unsafe static void Test(byte* pValue) { *pValue = (byte)Sse2.ConvertToInt32(Vector128.Create(-10, 0, 0, 0)); } From ac771be359dfbf91365e84a18b915d685d781ac1 Mon Sep 17 00:00:00 2001 From: Will Smith Date: Fri, 22 Sep 2023 14:29:14 -0700 Subject: [PATCH 9/9] Update Runtime_92349.cs --- .../Regression/JitBlue/Runtime_92349/Runtime_92349.cs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs index 79769714c394ab..5de0a28895b268 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_92349/Runtime_92349.cs @@ -19,8 +19,11 @@ unsafe static void Test(byte* pValue) [Fact] public unsafe static void EntryPoint() { - ulong value = 0; - Test((byte*)Unsafe.AsPointer(ref value)); - Assert.True(value == 246); + if (Sse2.IsSupported) + { + ulong value = 0; + Test((byte*)Unsafe.AsPointer(ref value)); + Assert.True(value == 246); + } } }