From fb02c97312dcc33637f44ccd7df03751b88d6741 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 10 Jul 2023 11:39:19 -0700 Subject: [PATCH 1/3] Ensure that the CpuId tests set preferredVectorByteLength to a non-zero value --- src/tests/JIT/HardwareIntrinsics/X86/X86Base/CpuId.cs | 6 +++++- src/tests/readytorun/HardwareIntrinsics/X86/CpuId.cs | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/tests/JIT/HardwareIntrinsics/X86/X86Base/CpuId.cs b/src/tests/JIT/HardwareIntrinsics/X86/X86Base/CpuId.cs index 954a7cb0f00e21..5fd971feb96709 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 605bfbfd99058e..40ba2d9a1f22e1 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)) From c51bf90798443b5ef1208ab26a304ecef9c0a758 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 10 Jul 2023 13:57:02 -0700 Subject: [PATCH 2/3] Ensure getPreferredVectorByteLength takes NAOT into account --- src/coreclr/jit/hwintrinsic.cpp | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/hwintrinsic.cpp b/src/coreclr/jit/hwintrinsic.cpp index ed58894a955436..a0496737343a52 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)) { From 113cbeec161ca2cda8c07dca473014abe9f90d01 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Tue, 11 Jul 2023 09:25:28 -0700 Subject: [PATCH 3/3] Don't condition the preferred vector byte length based on stress mode --- src/coreclr/jit/compiler.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 9129d5d4c3b27f..fb7f2f73351af9 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; } }