From 7bd46666716a0c7311b0e28594238fe4fa2d3b60 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Sat, 6 May 2023 09:09:26 -0700 Subject: [PATCH] Ensure getMaxSIMDStructBytes doesn't report `compVerifyInstructionSetUnusable` (#85370) --- .../coreclr/botr/vectors-and-intrinsics.md | 4 +- src/coreclr/jit/compiler.h | 73 +------------------ src/coreclr/jit/lclvars.cpp | 2 +- 3 files changed, 4 insertions(+), 75 deletions(-) diff --git a/docs/design/coreclr/botr/vectors-and-intrinsics.md b/docs/design/coreclr/botr/vectors-and-intrinsics.md index f507b55bd622f..fac5a0800c9be 100644 --- a/docs/design/coreclr/botr/vectors-and-intrinsics.md +++ b/docs/design/coreclr/botr/vectors-and-intrinsics.md @@ -194,7 +194,5 @@ While the above api exists, it is not expected that general purpose code within |`compExactlyDependsOn(isa)`| Use when making a decision to use or not use an instruction set when the decision will affect the semantics of the generated code. Should never be used in an assert. | Return whether or not an instruction set is supported. Calls notifyInstructionSetUsage with the result of that computation. |`compOpportunisticallyDependsOn(isa)`| Use when making an opportunistic decision to use or not use an instruction set. Use when the instruction set usage is a "nice to have optimization opportunity", but do not use when a false result may change the semantics of the program. Should never be used in an assert. | Return whether or not an instruction set is supported. Calls notifyInstructionSetUsage if the instruction set is supported. |`compIsaSupportedDebugOnly(isa)` | Use to assert whether or not an instruction set is supported | Return whether or not an instruction set is supported. Does not report anything. Only available in debug builds. -|`getSIMDVectorType()`| Use to get the TYP of a the `Vector` type. | Determine the TYP of the `Vector` type. If on the architecture the TYP may vary depending on whatever rules, this function will make sufficient use of the `notifyInstructionSetUsage` api to ensure that the TYP is consistent between compile time and runtime. -|`getSIMDVectorRegisterByteLength()` | Use to get the size of a `Vector` value. | Determine the size of the `Vector` type. If on the architecture the size may vary depending on whatever rules, this function will make sufficient use of the `notifyInstructionSetUsage` api to ensure that the size is consistent between compile time and runtime. +|`getSIMDVectorRegisterByteLength()` | Use to get the size of a `Vector` value. | Determine the size of the `Vector` type. If on the architecture the size may vary depending on whatever rules. Use `compExactlyDependsOn` to perform the queries so that the size is consistent between compile time and runtime. |`maxSIMDStructBytes()`| Get the maximum number of bytes that might be used in a SIMD type during this compilation. | Query the set of instruction sets supported, and determine the largest simd type supported. Use `compOpportunisticallyDependsOn` to perform the queries so that the maximum size needed is the only one recorded. -|`largestEnregisterableStructSize()`| Get the maximum number of bytes that might be represented by a single register in this compilation. Use only as an optimization to avoid calling `impNormStructType` or `getBaseTypeAndSizeOfSIMDType`. | Query the set of instruction sets supported, and determine the largest simd type supported in this compilation, report that size. diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index fe49473e7c7a7..6678a0a6d9f21 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -8656,29 +8656,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX bool areArgumentsContiguous(GenTree* op1, GenTree* op2); GenTree* CreateAddressNodeForSimdHWIntrinsicCreate(GenTree* tree, var_types simdBaseType, unsigned simdSize); - // Get the type for the hardware SIMD vector. - // This is the maximum SIMD type supported for this target. - var_types getSIMDVectorType() - { -#if defined(TARGET_XARCH) - if (compOpportunisticallyDependsOn(InstructionSet_AVX2)) - { - // TODO-XArch-AVX512 : Return TYP_SIMD64 once Vector supports AVX512. - return TYP_SIMD32; - } - else - { - compVerifyInstructionSetUnusable(InstructionSet_AVX2); - return TYP_SIMD16; - } -#elif defined(TARGET_ARM64) - return TYP_SIMD16; -#else - assert(!"getSIMDVectorType() unimplemented on target arch"); - unreached(); -#endif - } - // Get the size of the SIMD type in bytes int getSIMDTypeSizeInBytes(CORINFO_CLASS_HANDLE typeHnd) { @@ -8701,14 +8678,13 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX unsigned getSIMDVectorRegisterByteLength() { #if defined(TARGET_XARCH) - if (compOpportunisticallyDependsOn(InstructionSet_AVX2)) + if (compExactlyDependsOn(InstructionSet_AVX2)) { // TODO-XArch-AVX512 : Return ZMM_REGSIZE_BYTES once Vector supports AVX512. return YMM_REGSIZE_BYTES; } else { - compVerifyInstructionSetUnusable(InstructionSet_AVX2); return XMM_REGSIZE_BYTES; } #elif defined(TARGET_ARM64) @@ -8739,13 +8715,11 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX } else { - compVerifyInstructionSetUnusable(InstructionSet_AVX512F); return YMM_REGSIZE_BYTES; } } else { - compVerifyInstructionSetUnusable(InstructionSet_AVX); return XMM_REGSIZE_BYTES; } #elif defined(TARGET_ARM64) @@ -8969,44 +8943,12 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX return threshold; } - //------------------------------------------------------------------------ - // largestEnregisterableStruct: The size in bytes of the largest struct that can be enregistered. - // - // Notes: It is not guaranteed that the struct of this size or smaller WILL be a - // candidate for enregistration. - - unsigned largestEnregisterableStructSize() - { -#ifdef FEATURE_SIMD -#if defined(FEATURE_HW_INTRINSICS) && defined(TARGET_XARCH) - if (opts.IsReadyToRun()) - { - // Return constant instead of maxSIMDStructBytes, as maxSIMDStructBytes performs - // checks that are effected by the current level of instruction set support would - // otherwise cause the highest level of instruction set support to be reported to crossgen2. - // and this api is only ever used as an optimization or assert, so no reporting should - // ever happen. - return ZMM_REGSIZE_BYTES; - } -#endif // defined(FEATURE_HW_INTRINSICS) && defined(TARGET_XARCH) - unsigned vectorRegSize = maxSIMDStructBytes(); - assert(vectorRegSize >= TARGET_POINTER_SIZE); - return vectorRegSize; -#else // !FEATURE_SIMD - return TARGET_POINTER_SIZE; -#endif // !FEATURE_SIMD - } - // Use to determine if a struct *might* be a SIMD type. As this function only takes a size, many // structs will fit the criteria. bool structSizeMightRepresentSIMDType(size_t structSize) { #ifdef FEATURE_SIMD - // Do not use maxSIMDStructBytes as that api in R2R on X86 and X64 may notify the JIT - // about the size of a struct under the assumption that the struct size needs to be recorded. - // By using largestEnregisterableStructSize here, the detail of whether or not Vector256 is - // enregistered or not will not be messaged to the R2R compiler. - return (structSize >= minSIMDStructBytes()) && (structSize <= largestEnregisterableStructSize()); + return (structSize >= minSIMDStructBytes()) && (structSize <= maxSIMDStructBytes()); #else return false; #endif // FEATURE_SIMD @@ -9105,17 +9047,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX #endif } - // Ensure that code will not execute if an instruction set is usable. Call only - // if the instruction set has previously reported as unusable, but the status - // has not yet been recorded to the AOT compiler. - void compVerifyInstructionSetUnusable(CORINFO_InstructionSet isa) const - { - // use compExactlyDependsOn to capture are record the use of the ISA. - bool isaUsable = compExactlyDependsOn(isa); - // Assert that the is unusable. If true, this function should never be called. - assert(!isaUsable); - } - // Answer the question: Is a particular ISA allowed to be used implicitly by optimizations? // The result of this api call will match the target machine if the result is true. // If the result is false, then the target machine may have support for the instruction. diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 5bdac121889ef..60dc1828c1a4c 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -2241,7 +2241,7 @@ Compiler::lvaStructFieldInfo Compiler::StructPromotionHelper::GetFieldInfo(CORIN // We will only promote fields of SIMD types that fit into a SIMD register. if (simdBaseJitType != CORINFO_TYPE_UNDEF) { - if ((simdSize >= compiler->minSIMDStructBytes()) && (simdSize <= compiler->maxSIMDStructBytes())) + if (compiler->structSizeMightRepresentSIMDType(simdSize)) { fieldInfo.fldType = compiler->getSIMDTypeForSize(simdSize); fieldInfo.fldSize = simdSize;