From 8c2a5b153806b19db22795dc9c159467e78f79d1 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Wed, 19 May 2021 11:29:41 -0700 Subject: [PATCH 01/12] Fix an issue with Vector128.WithElement around unused nodes for pre SSE4.1 --- src/coreclr/jit/lowerxarch.cpp | 58 ++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index bbf7a72b6b5a43..7432bd23d6bc5c 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -3007,15 +3007,16 @@ void Lowering::LowerHWIntrinsicWithElement(GenTreeHWIntrinsic* node) NamedIntrinsic resIntrinsic = NI_Illegal; - idx = comp->gtNewIconNode(imm8); - BlockRange().InsertBefore(node, idx); - switch (simdBaseType) { case TYP_LONG: case TYP_ULONG: { - op2 = idx; + idx = comp->gtNewIconNode(imm8); + BlockRange().InsertBefore(node, idx); + + op1 = comp->gtNewArgList(op1, op3, idx); + op2 = nullptr; resIntrinsic = NI_SSE41_X64_Insert; break; } @@ -3033,7 +3034,7 @@ void Lowering::LowerHWIntrinsicWithElement(GenTreeHWIntrinsic* node) tmp1 = comp->gtNewSimdHWIntrinsicNode(TYP_SIMD16, op3, NI_Vector128_CreateScalarUnsafe, CORINFO_TYPE_FLOAT, 16); - BlockRange().InsertBefore(idx, tmp1); + BlockRange().InsertBefore(node, tmp1); LowerNode(tmp1); if (!comp->compOpportunisticallyDependsOn(InstructionSet_SSE41)) @@ -3139,8 +3140,8 @@ void Lowering::LowerHWIntrinsicWithElement(GenTreeHWIntrinsic* node) } else { - op3 = tmp1; - idx->AsIntCon()->SetIconValue(imm8 * 16); + imm8 = imm8 * 16; + op3 = tmp1; FALLTHROUGH; } } @@ -3150,6 +3151,9 @@ void Lowering::LowerHWIntrinsicWithElement(GenTreeHWIntrinsic* node) case TYP_INT: case TYP_UINT: { + idx = comp->gtNewIconNode(imm8); + BlockRange().InsertBefore(node, idx); + op1 = comp->gtNewArgList(op1, op3, idx); op2 = nullptr; resIntrinsic = NI_SSE41_Insert; @@ -3159,6 +3163,9 @@ void Lowering::LowerHWIntrinsicWithElement(GenTreeHWIntrinsic* node) case TYP_SHORT: case TYP_USHORT: { + idx = comp->gtNewIconNode(imm8); + BlockRange().InsertBefore(node, idx); + op1 = comp->gtNewArgList(op1, op3, idx); op2 = nullptr; resIntrinsic = NI_SSE2_Insert; @@ -3178,7 +3185,7 @@ void Lowering::LowerHWIntrinsicWithElement(GenTreeHWIntrinsic* node) tmp1 = comp->gtNewSimdHWIntrinsicNode(TYP_SIMD16, op3, NI_Vector128_CreateScalarUnsafe, CORINFO_TYPE_DOUBLE, 16); - BlockRange().InsertBefore(idx, tmp1); + BlockRange().InsertBefore(node, tmp1); LowerNode(tmp1); op2 = tmp1; @@ -5474,8 +5481,11 @@ bool Lowering::IsContainableHWIntrinsicOp(GenTreeHWIntrinsic* containingNode, Ge default: { - // These intrinsics only expect 16 or 32-byte nodes for containment - assert((genTypeSize(node->TypeGet()) == 16) || (genTypeSize(node->TypeGet()) == 32)); + if ((genTypeSize(node->TypeGet()) != 16) && (genTypeSize(node->TypeGet()) != 32)) + { + // These intrinsics only expect 16 or 32-byte nodes for containment + break; + } if (!comp->canUseVexEncoding()) { @@ -5535,8 +5545,11 @@ bool Lowering::IsContainableHWIntrinsicOp(GenTreeHWIntrinsic* containingNode, Ge case NI_AVX2_ShuffleHigh: case NI_AVX2_ShuffleLow: { - // These intrinsics only expect 16 or 32-byte nodes for containment - assert((genTypeSize(node->TypeGet()) == 16) || (genTypeSize(node->TypeGet()) == 32)); + if ((genTypeSize(node->TypeGet()) != 16) && (genTypeSize(node->TypeGet()) != 32)) + { + // These intrinsics only expect 16 or 32-byte nodes for containment + break; + } assert(supportsSIMDScalarLoads == false); supportsAlignedSIMDLoads = !comp->canUseVexEncoding() || !comp->opts.MinOpts(); @@ -5553,7 +5566,12 @@ bool Lowering::IsContainableHWIntrinsicOp(GenTreeHWIntrinsic* containingNode, Ge if (containingNode->GetSimdBaseType() == TYP_FLOAT) { assert(containingIntrinsicId == NI_SSE41_Insert); - assert(genTypeSize(node->TypeGet()) == 16); + + if (genTypeSize(node->TypeGet()) != 16) + { + // These intrinsics only expect 16-byte nodes for containment + break; + } // Sse41.Insert(V128, V128, byte) is a bit special // in that it has different behavior depending on whether the @@ -5620,8 +5638,11 @@ bool Lowering::IsContainableHWIntrinsicOp(GenTreeHWIntrinsic* containingNode, Ge case NI_AVX_CompareScalar: { - // These intrinsics only expect 16 or 32-byte nodes for containment - assert((genTypeSize(node->TypeGet()) == 16) || (genTypeSize(node->TypeGet()) == 32)); + if ((genTypeSize(node->TypeGet()) != 16) && (genTypeSize(node->TypeGet()) != 32)) + { + // These intrinsics only expect 16 or 32-byte nodes for containment + break; + } assert(supportsAlignedSIMDLoads == false); assert(supportsUnalignedSIMDLoads == false); @@ -5700,8 +5721,11 @@ bool Lowering::IsContainableHWIntrinsicOp(GenTreeHWIntrinsic* containingNode, Ge default: { - // These intrinsics only expect 16 or 32-byte nodes for containment - assert((genTypeSize(node->TypeGet()) == 16) || (genTypeSize(node->TypeGet()) == 32)); + if ((genTypeSize(node->TypeGet()) != 16) && (genTypeSize(node->TypeGet()) != 32)) + { + // These intrinsics only expect 16 or 32-byte nodes for containment + break; + } supportsSIMDScalarLoads = true; supportsGeneralLoads = supportsSIMDScalarLoads; From 241aa5d17f7654fd71dd4b59154185d99e0d2524 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Wed, 19 May 2021 14:03:16 -0700 Subject: [PATCH 02/12] Fixing the expected exception for a structreturn test --- src/tests/JIT/Directed/StructABI/structreturn.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/JIT/Directed/StructABI/structreturn.cs b/src/tests/JIT/Directed/StructABI/structreturn.cs index 72f54b306d8c36..cbcdd70a1e2184 100644 --- a/src/tests/JIT/Directed/StructABI/structreturn.cs +++ b/src/tests/JIT/Directed/StructABI/structreturn.cs @@ -1332,7 +1332,7 @@ private static void TestReturnViaThrowing() where T : struct T value = vector[Vector.Count]; System.Diagnostics.Debug.Assert(false); } - catch (IndexOutOfRangeException) + catch (ArgumentOutOfRangeException) { return; } From 765d1717f508ac9df224719453a13029b8b7834e Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 20 May 2021 09:42:12 -0700 Subject: [PATCH 03/12] Ensure we check if the baseline SIMD ISAs are supported in morph --- src/coreclr/jit/compiler.h | 17 +++++++++++++++++ src/coreclr/jit/morph.cpp | 4 ++-- src/coreclr/jit/simd.cpp | 2 +- src/coreclr/jit/simdashwintrinsic.cpp | 10 +--------- 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index b867331ff21103..168457d188ec65 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -8232,6 +8232,23 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX */ + bool IsBaselineSimdIsaSupported() + { +#ifdef FEATURE_SIMD +#if defined(TARGET_XARCH) + CORINFO_InstructionSet minimumIsa = InstructionSet_SSE2; +#elif defined(TARGET_ARM64) + CORINFO_InstructionSet minimumIsa = InstructionSet_AdvSimd; +#else +#error Unsupported platform +#endif // !TARGET_XARCH && !TARGET_ARM64 + + return compOpportunisticallyDependsOn(minimumIsa) && JitConfig.EnableHWIntrinsic(); +#else + return false; +#endif + } + // Get highest available level for SIMD codegen SIMDLevel getSIMDSupportLevel() { diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 2aac4cb13604eb..67ffb97ea74873 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -12044,7 +12044,7 @@ GenTree* Compiler::fgMorphFieldToSimdGetElement(GenTree* tree) unsigned simdSize = 0; GenTree* simdStructNode = getSIMDStructFromField(tree, &simdBaseJitType, &index, &simdSize); - if (simdStructNode != nullptr) + if ((simdStructNode != nullptr) && IsBaselineSimdIsaSupported()) { var_types simdBaseType = JitType2PreciseVarType(simdBaseJitType); GenTree* op2 = gtNewIconNode(index, TYP_INT); @@ -12081,7 +12081,7 @@ GenTree* Compiler::fgMorphFieldAssignToSimdSetElement(GenTree* tree) unsigned simdSize = 0; GenTree* simdStructNode = getSIMDStructFromField(tree->gtGetOp1(), &simdBaseJitType, &index, &simdSize); - if (simdStructNode != nullptr) + if ((simdStructNode != nullptr) && IsBaselineSimdIsaSupported()) { var_types simdType = simdStructNode->gtType; var_types simdBaseType = JitType2PreciseVarType(simdBaseJitType); diff --git a/src/coreclr/jit/simd.cpp b/src/coreclr/jit/simd.cpp index 0cb2947cff0de3..8ea52ce4117894 100644 --- a/src/coreclr/jit/simd.cpp +++ b/src/coreclr/jit/simd.cpp @@ -1909,7 +1909,7 @@ GenTree* Compiler::impSIMDIntrinsic(OPCODE opcode, #error Unsupported platform #endif // !TARGET_XARCH && !TARGET_ARM64 - if (!compOpportunisticallyDependsOn(minimumIsa) || !JitConfig.EnableHWIntrinsic()) + if (!IsBaselineSimdIsaSupported()) { // The user disabled support for the baseline ISA so // don't emit any SIMD intrinsics as they all require diff --git a/src/coreclr/jit/simdashwintrinsic.cpp b/src/coreclr/jit/simdashwintrinsic.cpp index 7369172c5c989c..e8f742cfc78a9a 100644 --- a/src/coreclr/jit/simdashwintrinsic.cpp +++ b/src/coreclr/jit/simdashwintrinsic.cpp @@ -176,15 +176,7 @@ GenTree* Compiler::impSimdAsHWIntrinsic(NamedIntrinsic intrinsic, return nullptr; } -#if defined(TARGET_XARCH) - CORINFO_InstructionSet minimumIsa = InstructionSet_SSE2; -#elif defined(TARGET_ARM64) - CORINFO_InstructionSet minimumIsa = InstructionSet_AdvSimd; -#else -#error Unsupported platform -#endif // !TARGET_XARCH && !TARGET_ARM64 - - if (!compOpportunisticallyDependsOn(minimumIsa) || !JitConfig.EnableHWIntrinsic()) + if (!IsBaselineSimdIsaSupported()) { // The user disabled support for the baseline ISA so // don't emit any SIMD intrinsics as they all require From d9bd42eaea423f7e9760dca7fb31cb7ef6265573 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 20 May 2021 10:44:00 -0700 Subject: [PATCH 04/12] Ensure TYP_SIMD12 LclVar can be cloned in lowering --- src/coreclr/jit/gentree.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 21e6f2f6eac9d2..0b164828d74afc 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6507,9 +6507,16 @@ GenTree* Compiler::gtNewLclvNode(unsigned lnum, var_types type DEBUGARG(IL_OFFSE // should be able to remove this exception and handle the assignment mismatch in // Lowering. LclVarDsc* varDsc = lvaGetDesc(lnum); +#if FEATURE_SIMD + // We can additionally have a SIMD12 that was widened to a SIMD16, generally as part of lowering + assert((type == varDsc->lvType) || ((type == TYP_SIMD16) && (varDsc->lvType == TYP_SIMD12)) || + (lvaIsImplicitByRefLocal(lnum) && fgGlobalMorph && (varDsc->lvType == TYP_BYREF)) || + ((varDsc->lvType == TYP_STRUCT) && (genTypeSize(type) == varDsc->lvExactSize))); +#else assert((type == varDsc->lvType) || (lvaIsImplicitByRefLocal(lnum) && fgGlobalMorph && (varDsc->lvType == TYP_BYREF)) || ((varDsc->lvType == TYP_STRUCT) && (genTypeSize(type) == varDsc->lvExactSize))); +#endif // FEATURE_SIMD } GenTree* node = new (this, GT_LCL_VAR) GenTreeLclVar(GT_LCL_VAR, type, lnum DEBUGARG(ILoffs)); From 1a4e854a3e4b5f171b0c00ebe4eb9eae75a49595 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 20 May 2021 11:38:12 -0700 Subject: [PATCH 05/12] Fixing up the non SSE41 path for WithElement --- src/coreclr/jit/lowerxarch.cpp | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index 7432bd23d6bc5c..1c4db4cc2a748d 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -3093,22 +3093,22 @@ void Lowering::LowerHWIntrinsicWithElement(GenTreeHWIntrinsic* node) { case 1: { - controlBits1 = 0; - controlBits2 = 226; + controlBits1 = 0; // 00 00 00 00; op1 = { X = op3, Y = op3, Z = op1.X, W = op1.X } + controlBits2 = 226; // 11 10 00 10; node = { X = op1.X, Y = op3, Z = op1.Z, W = op1.W } break; } case 2: { - controlBits1 = 48; - controlBits2 = 132; + controlBits1 = 15; // 00 00 11 11; op1 = { X = op1.W, Y = op1.W, Z = op3, W = op3 } + controlBits2 = 36; // 00 10 01 00; node = { X = op1.X, Y = op1.Y, Z = op3, W = op1.W } break; } case 3: { - controlBits1 = 32; - controlBits2 = 36; + controlBits1 = 10; // 00 00 10 10; op1 = { X = op1.Z, Y = op1.Z, Z = op3, W = op3 } + controlBits2 = 132; // 10 00 01 00; node = { X = op1.X, Y = op1.Y, Z = op1.Z, W = op3 } break; } @@ -3119,12 +3119,12 @@ void Lowering::LowerHWIntrinsicWithElement(GenTreeHWIntrinsic* node) idx = comp->gtNewIconNode(controlBits1); BlockRange().InsertAfter(tmp2, idx); - if (imm8 == 1) + if (imm8 != 1) { std::swap(tmp1, tmp2); } - op1 = comp->gtNewSimdHWIntrinsicNode(TYP_SIMD16, tmp2, tmp1, idx, NI_SSE_Shuffle, + op1 = comp->gtNewSimdHWIntrinsicNode(TYP_SIMD16, tmp1, tmp2, idx, NI_SSE_Shuffle, CORINFO_TYPE_FLOAT, 16); BlockRange().InsertAfter(idx, op1); LowerNode(op1); @@ -3132,6 +3132,11 @@ void Lowering::LowerHWIntrinsicWithElement(GenTreeHWIntrinsic* node) idx = comp->gtNewIconNode(controlBits2); BlockRange().InsertAfter(op1, idx); + if (imm8 != 1) + { + std::swap(op1, op2); + } + op1 = comp->gtNewArgList(op1, op2, idx); op2 = nullptr; resIntrinsic = NI_SSE_Shuffle; From b7c892656ad0a0b0dc1916cdadf187d71339c503 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 20 May 2021 12:39:01 -0700 Subject: [PATCH 06/12] Applying formatting patch --- src/coreclr/jit/gentree.cpp | 4 ++-- src/coreclr/jit/lowerxarch.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 0b164828d74afc..d1e54645dc5d71 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -7376,7 +7376,7 @@ GenTree* Compiler::gtNewPutArgReg(var_types type, GenTree* arg, regNumber argReg node->AsMultiRegOp()->gtOtherReg = REG_NEXT(argReg); } #else - node = gtNewOperNode(GT_PUTARG_REG, type, arg); + node = gtNewOperNode(GT_PUTARG_REG, type, arg); #endif node->SetRegNum(argReg); @@ -7406,7 +7406,7 @@ GenTree* Compiler::gtNewBitCastNode(var_types type, GenTree* arg) // A BITCAST could be a MultiRegOp on arm since we could move a double register to two int registers. node = new (this, GT_BITCAST) GenTreeMultiRegOp(GT_BITCAST, type, arg, nullptr); #else - node = gtNewOperNode(GT_BITCAST, type, arg); + node = gtNewOperNode(GT_BITCAST, type, arg); #endif return node; diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index 1c4db4cc2a748d..ab05fcc761ac3b 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -3100,8 +3100,8 @@ void Lowering::LowerHWIntrinsicWithElement(GenTreeHWIntrinsic* node) case 2: { - controlBits1 = 15; // 00 00 11 11; op1 = { X = op1.W, Y = op1.W, Z = op3, W = op3 } - controlBits2 = 36; // 00 10 01 00; node = { X = op1.X, Y = op1.Y, Z = op3, W = op1.W } + controlBits1 = 15; // 00 00 11 11; op1 = { X = op1.W, Y = op1.W, Z = op3, W = op3 } + controlBits2 = 36; // 00 10 01 00; node = { X = op1.X, Y = op1.Y, Z = op3, W = op1.W } break; } From d11612c2ddaea77556366f060f3e81e787831237 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 20 May 2021 15:07:26 -0700 Subject: [PATCH 07/12] Ensure ReplaceWithLclVar lowers the created LclVar and assignment --- src/coreclr/jit/lir.cpp | 7 ++++++- src/coreclr/jit/lir.h | 2 +- src/coreclr/jit/lower.h | 11 ++++++++++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/lir.cpp b/src/coreclr/jit/lir.cpp index beac0b069e2142..6bcc97b3ea2ea9 100644 --- a/src/coreclr/jit/lir.cpp +++ b/src/coreclr/jit/lir.cpp @@ -246,10 +246,11 @@ void LIR::Use::ReplaceWith(Compiler* compiler, GenTree* replacement) // lclNum - The local to use for temporary storage. If BAD_VAR_NUM (the // default) is provided, this method will create and use a new // local var. +// assign - On return, if non null, contains the created assignment node // // Return Value: The number of the local var used for temporary storage. // -unsigned LIR::Use::ReplaceWithLclVar(Compiler* compiler, unsigned lclNum) +unsigned LIR::Use::ReplaceWithLclVar(Compiler* compiler, unsigned lclNum, GenTree** assign) { assert(IsInitialized()); assert(compiler != nullptr); @@ -277,6 +278,10 @@ unsigned LIR::Use::ReplaceWithLclVar(Compiler* compiler, unsigned lclNum) JITDUMP("ReplaceWithLclVar created store :\n"); DISPNODE(store); + if (assign != nullptr) + { + *assign = store; + } return lclNum; } diff --git a/src/coreclr/jit/lir.h b/src/coreclr/jit/lir.h index 5348b9ed920e9b..e174482be6b36e 100644 --- a/src/coreclr/jit/lir.h +++ b/src/coreclr/jit/lir.h @@ -74,7 +74,7 @@ class LIR final bool IsDummyUse() const; void ReplaceWith(Compiler* compiler, GenTree* replacement); - unsigned ReplaceWithLclVar(Compiler* compiler, unsigned lclNum = BAD_VAR_NUM); + unsigned ReplaceWithLclVar(Compiler* compiler, unsigned lclNum = BAD_VAR_NUM, GenTree** assign = nullptr); }; //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index bc061bf7548f06..7f7c9d3760aa14 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -217,9 +217,18 @@ class Lowering final : public Phase GenTree* oldUseNode = use.Def(); if ((oldUseNode->gtOper != GT_LCL_VAR) || (tempNum != BAD_VAR_NUM)) { - use.ReplaceWithLclVar(comp, tempNum); + GenTree* assign; + use.ReplaceWithLclVar(comp, tempNum, &assign); + GenTree* newUseNode = use.Def(); ContainCheckRange(oldUseNode->gtNext, newUseNode); + + // We need to lower the LclVar and assignment since there may be certain + // types or scenarios, such as TYP_SIMD12, that need special handling + + LowerNode(assign); + LowerNode(newUseNode); + return newUseNode->AsLclVar(); } return oldUseNode->AsLclVar(); From 539e7ae076cb1d75800b8b96c5bd4988eb30bc84 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Thu, 20 May 2021 20:27:29 -0700 Subject: [PATCH 08/12] Don't check the JitLog for compiled methods when the baseline ISAs aren't supported --- src/tests/JIT/SIMD/VectorArray.cs | 81 ++++++++++++++++--------------- src/tests/JIT/SIMD/VectorGet.cs | 58 +++++++++++----------- 2 files changed, 73 insertions(+), 66 deletions(-) diff --git a/src/tests/JIT/SIMD/VectorArray.cs b/src/tests/JIT/SIMD/VectorArray.cs index 505743a7e8c72e..4de968286287f1 100644 --- a/src/tests/JIT/SIMD/VectorArray.cs +++ b/src/tests/JIT/SIMD/VectorArray.cs @@ -4,7 +4,8 @@ using System; using System.Numerics; - +using System.Runtime.Intrinsics.Arm; +using System.Runtime.Intrinsics.X86; internal partial class VectorTest { @@ -161,44 +162,46 @@ private static int Main() if (VectorArrayTest.VectorArray(1) != Pass) returnVal = Fail; if (VectorArrayTest.VectorArray(1) != Pass) returnVal = Fail; - JitLog jitLog = new JitLog(); - if (!jitLog.Check("get_Item", "Single")) returnVal = Fail; - if (!jitLog.Check("System.Numerics.Vector`1[Single][System.Single]:.ctor(float)")) returnVal = Fail; - if (!jitLog.Check("get_Item", "Double")) returnVal = Fail; - if (!jitLog.Check("System.Numerics.Vector`1[Double][System.Double]:.ctor(double)")) returnVal = Fail; - if (!jitLog.Check("get_Item", "Int32")) returnVal = Fail; - if (!jitLog.Check("System.Numerics.Vector`1[Int32][System.Int32]:.ctor(int)")) returnVal = Fail; - if (!jitLog.Check("get_Item", "Int64")) returnVal = Fail; - if (!jitLog.Check("System.Numerics.Vector`1[Int64][System.Int64]:.ctor(long)")) returnVal = Fail; - if (!jitLog.Check("System.Numerics.Vector4:.ctor(float)")) returnVal = Fail; - if (!jitLog.Check("System.Numerics.Vector3:.ctor(float)")) returnVal = Fail; - if (!jitLog.Check("System.Numerics.Vector2:.ctor(float)")) returnVal = Fail; - if (!jitLog.Check("get_Item", "UInt16")) returnVal = Fail; - // We are not currently recognizing the Vector constructor. - if (!Vector.IsHardwareAccelerated) - if (!jitLog.Check("System.Numerics.Vector`1[UInt16][System.UInt16]:.ctor(char)")) returnVal = Fail; - if (!jitLog.Check("get_Item", "Byte")) returnVal = Fail; - // We are not currently recognizing the Vector constructor. - if (!Vector.IsHardwareAccelerated) - if (!jitLog.Check("System.Numerics.Vector`1[Byte][System.Byte]:.ctor(ubyte)")) returnVal = Fail; - if (!jitLog.Check("get_Item", "Int16")) returnVal = Fail; - // We are not currently recognizing the Vector constructor. - if (!Vector.IsHardwareAccelerated) - if (!jitLog.Check("System.Numerics.Vector`1[Int16][System.Int16]:.ctor(short)")) returnVal = Fail; - if (!jitLog.Check("get_Item", "SByte")) returnVal = Fail; - // We are not currently recognizing the Vector constructor. - if (!Vector.IsHardwareAccelerated) - if (!jitLog.Check("System.Numerics.Vector`1[SByte][System.SByte]:.ctor(byte)")) returnVal = Fail; - if (!jitLog.Check("get_Item", "UInt32")) returnVal = Fail; - if (!jitLog.Check("System.Numerics.Vector`1[UInt32][System.UInt32]:.ctor(int)")) returnVal = Fail; - if (!jitLog.Check("get_Item", "UInt64")) returnVal = Fail; - if (!jitLog.Check("System.Numerics.Vector`1[UInt64][System.UInt64]:.ctor(long)")) returnVal = Fail; - if (!jitLog.Check("get_Item", "IntPtr")) returnVal = Fail; - if (!jitLog.Check("System.Numerics.Vector`1[IntPtr][System.UIntPtr]:.ctor(nuint)")) returnVal = Fail; - if (!jitLog.Check("get_Item", "UIntPtr")) returnVal = Fail; - if (!jitLog.Check("System.Numerics.Vector`1[UIntPtr][System.IntPtr]:.ctor(nint)")) returnVal = Fail; - jitLog.Dispose(); - + if (Sse41.IsSupported || AdvSimd.IsSupported) + { + JitLog jitLog = new JitLog(); + if (!jitLog.Check("get_Item", "Single")) returnVal = Fail; + if (!jitLog.Check("System.Numerics.Vector`1[Single][System.Single]:.ctor(float)")) returnVal = Fail; + if (!jitLog.Check("get_Item", "Double")) returnVal = Fail; + if (!jitLog.Check("System.Numerics.Vector`1[Double][System.Double]:.ctor(double)")) returnVal = Fail; + if (!jitLog.Check("get_Item", "Int32")) returnVal = Fail; + if (!jitLog.Check("System.Numerics.Vector`1[Int32][System.Int32]:.ctor(int)")) returnVal = Fail; + if (!jitLog.Check("get_Item", "Int64")) returnVal = Fail; + if (!jitLog.Check("System.Numerics.Vector`1[Int64][System.Int64]:.ctor(long)")) returnVal = Fail; + if (!jitLog.Check("System.Numerics.Vector4:.ctor(float)")) returnVal = Fail; + if (!jitLog.Check("System.Numerics.Vector3:.ctor(float)")) returnVal = Fail; + if (!jitLog.Check("System.Numerics.Vector2:.ctor(float)")) returnVal = Fail; + if (!jitLog.Check("get_Item", "UInt16")) returnVal = Fail; + // We are not currently recognizing the Vector constructor. + if (!Vector.IsHardwareAccelerated) + if (!jitLog.Check("System.Numerics.Vector`1[UInt16][System.UInt16]:.ctor(char)")) returnVal = Fail; + if (!jitLog.Check("get_Item", "Byte")) returnVal = Fail; + // We are not currently recognizing the Vector constructor. + if (!Vector.IsHardwareAccelerated) + if (!jitLog.Check("System.Numerics.Vector`1[Byte][System.Byte]:.ctor(ubyte)")) returnVal = Fail; + if (!jitLog.Check("get_Item", "Int16")) returnVal = Fail; + // We are not currently recognizing the Vector constructor. + if (!Vector.IsHardwareAccelerated) + if (!jitLog.Check("System.Numerics.Vector`1[Int16][System.Int16]:.ctor(short)")) returnVal = Fail; + if (!jitLog.Check("get_Item", "SByte")) returnVal = Fail; + // We are not currently recognizing the Vector constructor. + if (!Vector.IsHardwareAccelerated) + if (!jitLog.Check("System.Numerics.Vector`1[SByte][System.SByte]:.ctor(byte)")) returnVal = Fail; + if (!jitLog.Check("get_Item", "UInt32")) returnVal = Fail; + if (!jitLog.Check("System.Numerics.Vector`1[UInt32][System.UInt32]:.ctor(int)")) returnVal = Fail; + if (!jitLog.Check("get_Item", "UInt64")) returnVal = Fail; + if (!jitLog.Check("System.Numerics.Vector`1[UInt64][System.UInt64]:.ctor(long)")) returnVal = Fail; + if (!jitLog.Check("get_Item", "IntPtr")) returnVal = Fail; + if (!jitLog.Check("System.Numerics.Vector`1[IntPtr][System.UIntPtr]:.ctor(nuint)")) returnVal = Fail; + if (!jitLog.Check("get_Item", "UIntPtr")) returnVal = Fail; + if (!jitLog.Check("System.Numerics.Vector`1[UIntPtr][System.IntPtr]:.ctor(nint)")) returnVal = Fail; + jitLog.Dispose(); + } } catch (ArgumentException ex) { diff --git a/src/tests/JIT/SIMD/VectorGet.cs b/src/tests/JIT/SIMD/VectorGet.cs index d46e98c6533ac9..aa9fb118eb2ba1 100644 --- a/src/tests/JIT/SIMD/VectorGet.cs +++ b/src/tests/JIT/SIMD/VectorGet.cs @@ -5,6 +5,8 @@ using System; using System.Numerics; using System.Runtime.CompilerServices; +using System.Runtime.Intrinsics.Arm; +using System.Runtime.Intrinsics.X86; internal partial class VectorTest { @@ -206,33 +208,35 @@ private static int Main() if (VectorGetTest.VectorGet(100, 1) == Fail) returnVal = Fail; if (VectorGetTest.VectorGetIndexerOutOfRange(100, 1) == Fail) returnVal = Fail; - JitLog jitLog = new JitLog(); - if (!jitLog.Check("get_Item", "Double")) returnVal = Fail; - if (!jitLog.Check("get_Count", "Double")) returnVal = Fail; - if (!jitLog.Check("get_Item", "Single")) returnVal = Fail; - if (!jitLog.Check("get_Count", "Single")) returnVal = Fail; - if (!jitLog.Check("get_Item", "Int32")) returnVal = Fail; - if (!jitLog.Check("get_Count", "Int32")) returnVal = Fail; - if (!jitLog.Check("get_Item", "Int64")) returnVal = Fail; - if (!jitLog.Check("get_Count", "Int64")) returnVal = Fail; - if (!jitLog.Check("get_Item", "UInt16")) returnVal = Fail; - if (!jitLog.Check("get_Count", "UInt16")) returnVal = Fail; - if (!jitLog.Check("get_Item", "Byte")) returnVal = Fail; - if (!jitLog.Check("get_Count", "Byte")) returnVal = Fail; - if (!jitLog.Check("get_Item", "Int16")) returnVal = Fail; - if (!jitLog.Check("get_Count", "Int16")) returnVal = Fail; - if (!jitLog.Check("get_Item", "SByte")) returnVal = Fail; - if (!jitLog.Check("get_Count", "SByte")) returnVal = Fail; - if (!jitLog.Check("get_Item", "UInt32")) returnVal = Fail; - if (!jitLog.Check("get_Count", "UInt32")) returnVal = Fail; - if (!jitLog.Check("get_Item", "UInt64")) returnVal = Fail; - if (!jitLog.Check("get_Count", "UInt64")) returnVal = Fail; - if (!jitLog.Check("get_Item", "IntPtr")) returnVal = Fail; - if (!jitLog.Check("get_Count", "IntPtr")) returnVal = Fail; - if (!jitLog.Check("get_Item", "UIntPtr")) returnVal = Fail; - if (!jitLog.Check("get_Count", "UIntPtr")) returnVal = Fail; - jitLog.Dispose(); - + if (Sse41.IsSupported || AdvSimd.IsSupported) + { + JitLog jitLog = new JitLog(); + if (!jitLog.Check("get_Item", "Double")) returnVal = Fail; + if (!jitLog.Check("get_Count", "Double")) returnVal = Fail; + if (!jitLog.Check("get_Item", "Single")) returnVal = Fail; + if (!jitLog.Check("get_Count", "Single")) returnVal = Fail; + if (!jitLog.Check("get_Item", "Int32")) returnVal = Fail; + if (!jitLog.Check("get_Count", "Int32")) returnVal = Fail; + if (!jitLog.Check("get_Item", "Int64")) returnVal = Fail; + if (!jitLog.Check("get_Count", "Int64")) returnVal = Fail; + if (!jitLog.Check("get_Item", "UInt16")) returnVal = Fail; + if (!jitLog.Check("get_Count", "UInt16")) returnVal = Fail; + if (!jitLog.Check("get_Item", "Byte")) returnVal = Fail; + if (!jitLog.Check("get_Count", "Byte")) returnVal = Fail; + if (!jitLog.Check("get_Item", "Int16")) returnVal = Fail; + if (!jitLog.Check("get_Count", "Int16")) returnVal = Fail; + if (!jitLog.Check("get_Item", "SByte")) returnVal = Fail; + if (!jitLog.Check("get_Count", "SByte")) returnVal = Fail; + if (!jitLog.Check("get_Item", "UInt32")) returnVal = Fail; + if (!jitLog.Check("get_Count", "UInt32")) returnVal = Fail; + if (!jitLog.Check("get_Item", "UInt64")) returnVal = Fail; + if (!jitLog.Check("get_Count", "UInt64")) returnVal = Fail; + if (!jitLog.Check("get_Item", "IntPtr")) returnVal = Fail; + if (!jitLog.Check("get_Count", "IntPtr")) returnVal = Fail; + if (!jitLog.Check("get_Item", "UIntPtr")) returnVal = Fail; + if (!jitLog.Check("get_Count", "UIntPtr")) returnVal = Fail; + jitLog.Dispose(); + } return returnVal; } } From 1616c162a747c1064bf156ad7cc749c07674213a Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 24 May 2021 15:09:25 -0700 Subject: [PATCH 09/12] Address PR feedback --- src/coreclr/jit/gentree.cpp | 11 +++++------ src/coreclr/jit/simd.cpp | 8 -------- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index d1e54645dc5d71..48f7a1689d72f0 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6507,16 +6507,15 @@ GenTree* Compiler::gtNewLclvNode(unsigned lnum, var_types type DEBUGARG(IL_OFFSE // should be able to remove this exception and handle the assignment mismatch in // Lowering. LclVarDsc* varDsc = lvaGetDesc(lnum); + + bool simd12ToSimd16Widening = false; #if FEATURE_SIMD // We can additionally have a SIMD12 that was widened to a SIMD16, generally as part of lowering - assert((type == varDsc->lvType) || ((type == TYP_SIMD16) && (varDsc->lvType == TYP_SIMD12)) || - (lvaIsImplicitByRefLocal(lnum) && fgGlobalMorph && (varDsc->lvType == TYP_BYREF)) || - ((varDsc->lvType == TYP_STRUCT) && (genTypeSize(type) == varDsc->lvExactSize))); -#else - assert((type == varDsc->lvType) || + simd12ToSimd16Widening = (type == TYP_SIMD16) && (varDsc->lvType == TYP_SIMD12); +#endif + assert((type == varDsc->lvType) || simd12ToSimd16Widening || (lvaIsImplicitByRefLocal(lnum) && fgGlobalMorph && (varDsc->lvType == TYP_BYREF)) || ((varDsc->lvType == TYP_STRUCT) && (genTypeSize(type) == varDsc->lvExactSize))); -#endif // FEATURE_SIMD } GenTree* node = new (this, GT_LCL_VAR) GenTreeLclVar(GT_LCL_VAR, type, lnum DEBUGARG(ILoffs)); diff --git a/src/coreclr/jit/simd.cpp b/src/coreclr/jit/simd.cpp index 8ea52ce4117894..2eb0fca532ca85 100644 --- a/src/coreclr/jit/simd.cpp +++ b/src/coreclr/jit/simd.cpp @@ -1901,14 +1901,6 @@ GenTree* Compiler::impSIMDIntrinsic(OPCODE opcode, return nullptr; } -#if defined(TARGET_XARCH) - CORINFO_InstructionSet minimumIsa = InstructionSet_SSE2; -#elif defined(TARGET_ARM64) - CORINFO_InstructionSet minimumIsa = InstructionSet_AdvSimd; -#else -#error Unsupported platform -#endif // !TARGET_XARCH && !TARGET_ARM64 - if (!IsBaselineSimdIsaSupported()) { // The user disabled support for the baseline ISA so From ff9cc2700acdb132f282ecd9b386e89cdafdebc1 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 24 May 2021 17:44:22 -0700 Subject: [PATCH 10/12] Responding to more PR feedback --- src/coreclr/jit/lowerxarch.cpp | 12 +++++++++++- src/coreclr/jit/morph.cpp | 30 +++++++++++++++++------------- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index ab05fcc761ac3b..2c104482d30f0a 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -3089,6 +3089,16 @@ void Lowering::LowerHWIntrinsicWithElement(GenTreeHWIntrinsic* node) ssize_t controlBits1; ssize_t controlBits2; + // The comments beside the control bits below are listed using the managed API operands + // + // In practice, for the first step the value being inserted (op3) is in tmp1 + // while the other elements of the result (op1) are in tmp2. The result ends + // up containing the value being inserted and its immediate neighbor. + // + // The second step takes that result (which is in op1) plus the other elements + // from op2 (a clone of op1/tmp2 from the previous step) and combines them to + // create the final result. + switch (imm8) { case 1: @@ -5555,7 +5565,7 @@ bool Lowering::IsContainableHWIntrinsicOp(GenTreeHWIntrinsic* containingNode, Ge // These intrinsics only expect 16 or 32-byte nodes for containment break; } - assert(supportsSIMDScalarLoads == false); + assert(!supportsSIMDScalarLoads); supportsAlignedSIMDLoads = !comp->canUseVexEncoding() || !comp->opts.MinOpts(); supportsUnalignedSIMDLoads = comp->canUseVexEncoding(); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 67ffb97ea74873..c15e515aca8134 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -6011,22 +6011,25 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) } #ifdef FEATURE_SIMD - // if this field belongs to simd struct, translate it to simd intrinsic. - if (mac == nullptr) + if (IsBaselineSimdIsaSupported()) { - GenTree* newTree = fgMorphFieldToSimdGetElement(tree); - if (newTree != tree) + // if this field belongs to simd struct, translate it to simd intrinsic. + if (mac == nullptr) { - newTree = fgMorphSmpOp(newTree); - return newTree; + GenTree* newTree = fgMorphFieldToSimdGetElement(tree); + if (newTree != tree) + { + newTree = fgMorphSmpOp(newTree); + return newTree; + } } - } - else if ((objRef != nullptr) && (objRef->OperGet() == GT_ADDR) && varTypeIsSIMD(objRef->gtGetOp1())) - { - GenTreeLclVarCommon* lcl = objRef->IsLocalAddrExpr(); - if (lcl != nullptr) + else if ((objRef != nullptr) && (objRef->OperGet() == GT_ADDR) && varTypeIsSIMD(objRef->gtGetOp1())) { - lvaSetVarDoNotEnregister(lcl->GetLclNum() DEBUGARG(DNER_LocalField)); + GenTreeLclVarCommon* lcl = objRef->IsLocalAddrExpr(); + if (lcl != nullptr) + { + lvaSetVarDoNotEnregister(lcl->GetLclNum() DEBUGARG(DNER_LocalField)); + } } } #endif @@ -12044,7 +12047,7 @@ GenTree* Compiler::fgMorphFieldToSimdGetElement(GenTree* tree) unsigned simdSize = 0; GenTree* simdStructNode = getSIMDStructFromField(tree, &simdBaseJitType, &index, &simdSize); - if ((simdStructNode != nullptr) && IsBaselineSimdIsaSupported()) + if (simdStructNode != nullptr) { var_types simdBaseType = JitType2PreciseVarType(simdBaseJitType); GenTree* op2 = gtNewIconNode(index, TYP_INT); @@ -12243,6 +12246,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) op2 = tree->AsOp()->gtOp2; #ifdef FEATURE_SIMD + if (IsBaselineSimdIsaSupported()) { // We should check whether op2 should be assigned to a SIMD field or not. // If it is, we should tranlate the tree to simd intrinsic. From 1b41e6f5e594dfcf75f4a535c46a7d12794b896b Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 24 May 2021 18:04:48 -0700 Subject: [PATCH 11/12] Applying formatting patch --- src/coreclr/jit/gentree.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 50e07ce00eb48a..8c3a669d3568b5 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -7375,7 +7375,7 @@ GenTree* Compiler::gtNewPutArgReg(var_types type, GenTree* arg, regNumber argReg node->AsMultiRegOp()->gtOtherReg = REG_NEXT(argReg); } #else - node = gtNewOperNode(GT_PUTARG_REG, type, arg); + node = gtNewOperNode(GT_PUTARG_REG, type, arg); #endif node->SetRegNum(argReg); @@ -7405,7 +7405,7 @@ GenTree* Compiler::gtNewBitCastNode(var_types type, GenTree* arg) // A BITCAST could be a MultiRegOp on arm since we could move a double register to two int registers. node = new (this, GT_BITCAST) GenTreeMultiRegOp(GT_BITCAST, type, arg, nullptr); #else - node = gtNewOperNode(GT_BITCAST, type, arg); + node = gtNewOperNode(GT_BITCAST, type, arg); #endif return node; From 4f08fc6e8155dc9994673d3554d71d8b11b9853c Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Tue, 25 May 2021 10:01:48 -0700 Subject: [PATCH 12/12] Fixing the more PR review feedback --- src/coreclr/jit/morph.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 8244036ea58b14..b6e1f808ea9094 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -6006,10 +6006,10 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) } #ifdef FEATURE_SIMD - if (IsBaselineSimdIsaSupported()) + // if this field belongs to simd struct, translate it to simd intrinsic. + if (mac == nullptr) { - // if this field belongs to simd struct, translate it to simd intrinsic. - if (mac == nullptr) + if (IsBaselineSimdIsaSupported()) { GenTree* newTree = fgMorphFieldToSimdGetElement(tree); if (newTree != tree) @@ -6018,13 +6018,13 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) return newTree; } } - else if ((objRef != nullptr) && (objRef->OperGet() == GT_ADDR) && varTypeIsSIMD(objRef->gtGetOp1())) + } + else if ((objRef != nullptr) && (objRef->OperGet() == GT_ADDR) && varTypeIsSIMD(objRef->gtGetOp1())) + { + GenTreeLclVarCommon* lcl = objRef->IsLocalAddrExpr(); + if (lcl != nullptr) { - GenTreeLclVarCommon* lcl = objRef->IsLocalAddrExpr(); - if (lcl != nullptr) - { - lvaSetVarDoNotEnregister(lcl->GetLclNum() DEBUGARG(DNER_LocalField)); - } + lvaSetVarDoNotEnregister(lcl->GetLclNum() DEBUGARG(DNER_LocalField)); } } #endif @@ -12079,7 +12079,7 @@ GenTree* Compiler::fgMorphFieldAssignToSimdSetElement(GenTree* tree) unsigned simdSize = 0; GenTree* simdStructNode = getSIMDStructFromField(tree->gtGetOp1(), &simdBaseJitType, &index, &simdSize); - if ((simdStructNode != nullptr) && IsBaselineSimdIsaSupported()) + if (simdStructNode != nullptr) { var_types simdType = simdStructNode->gtType; var_types simdBaseType = JitType2PreciseVarType(simdBaseJitType);