From 83bf4b66ed3de5905b2e410a194a0f5f3d924c3d Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Tue, 11 Jul 2023 15:26:36 -0700 Subject: [PATCH] Ensure that the CpuId tests set preferredVectorByteLength to a non-zero value (#88623) * Ensure that the CpuId tests set preferredVectorByteLength to a non-zero value * Ensure getPreferredVectorByteLength takes NAOT into account * Don't condition the preferred vector byte length based on stress mode --- src/coreclr/jit/compiler.cpp | 9 +++---- src/coreclr/jit/hwintrinsic.cpp | 26 +++++++++---------- .../HardwareIntrinsics/X86/X86Base/CpuId.cs | 6 ++++- .../HardwareIntrinsics/X86/CpuId.cs | 6 ++++- 4 files changed, 25 insertions(+), 22 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 9129d5d4c3b27..fb7f2f73351af 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -2306,13 +2306,10 @@ void Compiler::compSetProcessor() // users can override this with `DOTNET_PreferredVectorBitWidth=512` to // allow using such instructions where hardware support is available. // - // Under stress, sometimes leave the preferred vector width at 512, even if that means - // throttling. This helps with test coverage on test machines that might be older. + // Do not condition this based on stress mode as it makes the support + // reported inconsistent across methods and breaks expectations/functionality - if (!compStressCompile(STRESS_GENERIC_VARN, 20)) - { - preferredVectorByteLength = 256 / 8; - } + preferredVectorByteLength = 256 / 8; } } diff --git a/src/coreclr/jit/hwintrinsic.cpp b/src/coreclr/jit/hwintrinsic.cpp index ed58894a95543..a0496737343a5 100644 --- a/src/coreclr/jit/hwintrinsic.cpp +++ b/src/coreclr/jit/hwintrinsic.cpp @@ -495,8 +495,9 @@ NamedIntrinsic HWIntrinsicInfo::lookupId(Compiler* comp, return NI_Illegal; } - bool isIsaSupported = comp->compSupportsHWIntrinsic(isa); - bool isHardwareAcceleratedProp = (strcmp(methodName, "get_IsHardwareAccelerated") == 0); + bool isIsaSupported = comp->compSupportsHWIntrinsic(isa); + bool isHardwareAcceleratedProp = (strcmp(methodName, "get_IsHardwareAccelerated") == 0); + uint32_t vectorByteLength = 0; #ifdef TARGET_XARCH if (isHardwareAcceleratedProp) @@ -505,25 +506,21 @@ NamedIntrinsic HWIntrinsicInfo::lookupId(Compiler* comp, // but we want IsHardwareAccelerated to return true only when all of them are (there are // still can be cases where e.g. Sse41 might give an additional boost for Vector128, but it's // not important enough to bump the minimal Sse version here) + if (strcmp(className, "Vector128") == 0) { - isa = InstructionSet_SSE2; + isa = InstructionSet_SSE2; + vectorByteLength = 16; } else if (strcmp(className, "Vector256") == 0) { - if (comp->getPreferredVectorByteLength() < 32) - { - return NI_IsSupported_False; - } - isa = InstructionSet_AVX2; + isa = InstructionSet_AVX2; + vectorByteLength = 32; } else if (strcmp(className, "Vector512") == 0) { - if (comp->getPreferredVectorByteLength() < 64) - { - return NI_IsSupported_False; - } - isa = InstructionSet_AVX512F; + isa = InstructionSet_AVX512F; + vectorByteLength = 64; } } #endif @@ -556,7 +553,8 @@ NamedIntrinsic HWIntrinsicInfo::lookupId(Compiler* comp, // When the compiler doesn't support ISA or when it does but the target hardware does // not and we aren't in a scenario with support for a dynamic check, we want to return false. - if (isIsaSupported && comp->compSupportsHWIntrinsic(isa)) + if (isIsaSupported && comp->compSupportsHWIntrinsic(isa) && + (vectorByteLength <= comp->getPreferredVectorByteLength())) { if (!comp->IsTargetAbi(CORINFO_NATIVEAOT_ABI) || comp->compExactlyDependsOn(isa)) { diff --git a/src/tests/JIT/HardwareIntrinsics/X86/X86Base/CpuId.cs b/src/tests/JIT/HardwareIntrinsics/X86/X86Base/CpuId.cs index 954a7cb0f00e2..5fd971feb9670 100644 --- a/src/tests/JIT/HardwareIntrinsics/X86/X86Base/CpuId.cs +++ b/src/tests/JIT/HardwareIntrinsics/X86/X86Base/CpuId.cs @@ -256,10 +256,14 @@ public unsafe static void CpuId() } } - if (isVector512Throttling) + if (isAvx512HierarchyDisabled || isVector512Throttling) { preferredVectorByteLength = 256 / 8; } + else + { + preferredVectorByteLength = 512 / 8; + } } if (IsBitIncorrect(ecx, 1, typeof(Avx512Vbmi), Avx512Vbmi.IsSupported, "AVX512VBMI", ref isHierarchyDisabled)) diff --git a/src/tests/readytorun/HardwareIntrinsics/X86/CpuId.cs b/src/tests/readytorun/HardwareIntrinsics/X86/CpuId.cs index 605bfbfd99058..40ba2d9a1f22e 100644 --- a/src/tests/readytorun/HardwareIntrinsics/X86/CpuId.cs +++ b/src/tests/readytorun/HardwareIntrinsics/X86/CpuId.cs @@ -251,10 +251,14 @@ public unsafe static int Main() } } - if (isVector512Throttling) + if (isAvx512HierarchyDisabled || isVector512Throttling) { preferredVectorByteLength = 256 / 8; } + else + { + preferredVectorByteLength = 512 / 8; + } } if (IsBitIncorrect(ecx, 1, typeof(Avx512Vbmi), Avx512Vbmi.IsSupported, "AVX512VBMI", ref isHierarchyDisabled))