Adding CPUID for AVX10.2 #109302

khushal1996 · 2024-10-28T19:15:20Z

Overview

This PR tracks addition of CPUID for AVX10.2 to runtime. We are following the spec doc to add the CPUID for AVX10.2. Following things have been modified as a part of this PR

Adding a new ISA InstructionSet_Avx10v2 and InstructionSet_Avx10v2_V512
Adding a new environment variable EnableAvx10v2 to JIT.
Handle enabling/disabling of AVX10 ISA in a careful manner. EnableAvx10v1 will take priority over EnableAvx10v2 i.e. if Avx10v1 is disabled, Avx10v2 will be disabled too.
Remove checks for CPUID for AVX10v1_V128 since bit 17 will imply V128 and V256 support i.e. minimum support on AVX10.

Testing Correctness of Logic

Since we don't have the DMR hardware available right now, the testing for this PR was done on internal SDE. Following scenarios were tested on internal SDE

Add debug prints to check the value of cpufeatures after setting all the ISA flags.
Enable AVX10v1 and AVX10v2 by default and do the same.
Disable Avx10v1 and check if it disables Avx10v2. (it should disable AVX10v2)
Disable Avx10v2 and verify that Avx10v1 stays enabled.

Testing JIT test suite

The testing was done using the Rex2 unit testing framework. All the changes made for testing CPUID changes are present in this repo

A brief overview -->

Unit tests ran to make sure no existing test is breaking
SDE embedded in test framework
Test ran with Intel internal SDE using dmr machine

Step 1: Run all tests without SDE

Step 2: Running all tests on the current branch with sde -dmr and DOTNET_EnableAVX10v2=0

Step 3: Running all tests on the current branch with sde -dmr

Testing ASMDIFFS

Using jit-diff method, this testing step compares assembly between the main and the current branch and makes sure that we are not changing any existing scenarios.

Note - we cannot use superpmi here since we are adding a new CPUID and hence the jiteeversionguid has changed which would lead to wrong results from superpmi.

dotnet-policy-service · 2024-10-28T19:15:56Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

src/coreclr/tools/Common/JitInterface/CorInfoInstructionSet.cs

khushal1996 · 2024-11-13T00:00:31Z

@tannergooding Can you please help review this PR?

tannergooding · 2024-11-18T20:26:51Z

This has conflicts that need resolving now that #104637 is merged

khushal1996 · 2024-11-18T22:27:07Z

@tannergooding PR has been rebased and updated.

src/coreclr/jit/hwintrinsic.cpp

src/coreclr/vm/codeman.cpp

tannergooding · 2024-11-19T18:07:14Z

I think we're missing some handling in:

src/coreclr/tools/Common/Compiler/InstructionSetSupport.cs
src/coreclr/tools/Common/InstructionSetHelpers.cs
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/HardwareIntrinsicHelpers.Aot.cs

Since we don't have managed ISAs yet we don't need to handle:

src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.NoX86Intrinsics.xml
src/mono/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Intrinsics.x86.xml
src/mono/mono/mini/simd-intrinsics.c

khushal1996 · 2024-11-19T20:06:35Z

I think we're missing some handling in:

src/coreclr/tools/Common/Compiler/InstructionSetSupport.cs

src/coreclr/tools/Common/InstructionSetHelpers.cs

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/HardwareIntrinsicHelpers.Aot.cs

Since we don't have managed ISAs yet we don't need to handle:

src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.NoX86Intrinsics.xml

src/mono/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Intrinsics.x86.xml

src/mono/mono/mini/simd-intrinsics.c

Thanks for pointing me to this. Looks like this handling was added later to handle AVX10v1 support. I have updated the handling for AVX10v2 accordingly.

tannergooding · 2024-11-19T20:55:51Z

CC, @dotnet/jit-contrib for secondary review

src/coreclr/scripts/out.txt

BruceForstall · 2024-11-20T00:21:35Z

src/coreclr/tools/Common/Compiler/InstructionSetSupport.cs

@@ -375,6 +375,10 @@ public bool ComputeInstructionSetFlags(int maxVectorTBitWidth,

                if (_supportedInstructionSets.Contains("avx10v1"))
                    _supportedInstructionSets.Add("avx10v1_v512");
+
+                // Having AVX10V1-V512 enabled, means having AVX10V2-V512 enabled as well.


Is this backwards? It seems that if avx10v2_v512 is supported then avx10v1_v512 is also supported, but not vice-versa.

This one does look incorrect. I believe it should be if (_supportedInstructionSets.Contains("avx10v2")) so it matches the AVX10v1 handling just above.

Which is to say that if Avx10v2 is supported and AVX512 is supported, then Avx10v2_V512 is supported.

I had deliberately kept this as is. By implication

implication ,X86 ,AVX10v2 ,AVX10v1
implication ,X86 ,AVX10v2_V512 ,AVX10v1_V512

And what bruce is saying is correct. If we have AVX10v2_V512, then we would have AVX10v1_V512. Please correct me if I am understanding it wrong. From the design, we cannot have AVX10v1 and have AVX10v2. But to maintain a standard code, I will just change it to what we have for now.

Pushed this change.

The code is doing functionally implication ,X86 ,AVX10v1_V512, AVX10v2_V512 where-as the actual implication is implication ,X86 ,AVX10v2_V512, AVX10v1_V512. This would result in a machine/target with only AVX10v1 enabling AVX10v2 support.

The way the logic in this function is meant to work is to cover implications that cannot be tracked trivially by the hierarchy, such as enabling AVX10v2.V512 when AVX10v2 + AVX512 is supported.

Makes sense. thanks for pointing this out @BruceForstall @tannergooding

BruceForstall · 2024-11-20T00:24:00Z

src/coreclr/tools/Common/JitInterface/CorInfoInstructionSet.cs

+                    if (resultflags.HasInstructionSet(InstructionSet.X86_AVX10v1))
+                        resultflags.AddInstructionSet(InstructionSet.X86_AVX10v2);
+                    if (resultflags.HasInstructionSet(InstructionSet.X86_AVX10v1_V512))
+                        resultflags.AddInstructionSet(InstructionSet.X86_AVX10v2_V512);


This seems odd: it says that if AVX10v1 is supported, then AVX10v2 is supported. Is that correct? I assume the OPPOSITE is true.

This is part of ExpandInstructionSetByReverseImplicationHelper, which exists to allow NAOT to enable any dependent ISAs if the user just specifies avx10v2 on the command line.

The opposite is true. @khushal1996 please correct this.

@anthonycanino this part is correct (and is notably autogenerated by the tool). Its part of ExpandInstructionSetByReverseImplicationHelper

BruceForstall · 2024-11-20T21:29:57Z

@khushal1996 A change that added another ISA just went in, meaning you need to resolve the merge conflicts in this PR.

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 28, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 28, 2024

khushal1996 force-pushed the kcm-avx102-cpuid branch from edcaabd to a55717e Compare October 28, 2024 19:19

khushal1996 marked this pull request as ready for review October 29, 2024 16:15

khushal1996 requested a review from MichalStrehovsky as a code owner October 29, 2024 16:15

saucecontrol reviewed Oct 29, 2024

View reviewed changes

src/coreclr/tools/Common/JitInterface/CorInfoInstructionSet.cs Outdated Show resolved Hide resolved

This was referenced Oct 29, 2024

chrome-DebuggerTests timing out #108078

Open

[debugger] Inspector exceptions in debugger tests #109025

Open

khushal1996 force-pushed the kcm-avx102-cpuid branch from fe57968 to 738cff9 Compare November 12, 2024 23:43

khushal1996 closed this Nov 18, 2024

khushal1996 force-pushed the kcm-avx102-cpuid branch from 738cff9 to e1aa9cf Compare November 18, 2024 22:05

khushal1996 reopened this Nov 18, 2024

saucecontrol reviewed Nov 18, 2024

View reviewed changes

src/coreclr/jit/hwintrinsic.cpp Outdated Show resolved Hide resolved

tannergooding reviewed Nov 19, 2024

View reviewed changes

src/coreclr/vm/codeman.cpp Outdated Show resolved Hide resolved

tannergooding approved these changes Nov 19, 2024

View reviewed changes

BruceForstall reviewed Nov 20, 2024

View reviewed changes

src/coreclr/scripts/out.txt Outdated Show resolved Hide resolved

BruceForstall reviewed Nov 20, 2024

View reviewed changes

khushal1996 added 4 commits November 20, 2024 13:57

Add CPUID for AVX10.2

1d1317a

Handle AVX10.2 ISAs in missing places

0208162

Remove APX_X64 since it is unused

7f11284

remove log file and change handling for AVX10v2_V512

ac909d2

build-analysis bot mentioned this pull request Nov 20, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

resolve merge errors

0eaa744

khushal1996 force-pushed the kcm-avx102-cpuid branch from 910b644 to 0eaa744 Compare November 20, 2024 22:47

This was referenced Nov 21, 2024

[QUIC & HTTP/3] Handshake Timeout on tests #104426

Open

System.Net.Quic.Tests.MsQuicTests.WriteTests failed with "System.Net.Quic.QuicException : The connection timed out from inactivity." #105177

Open

BruceForstall approved these changes Nov 21, 2024

View reviewed changes

BruceForstall merged commit 5345dc5 into dotnet:main Nov 21, 2024
159 of 162 checks passed

khushal1996 deleted the kcm-avx102-cpuid branch November 21, 2024 15:09

khushal1996 restored the kcm-avx102-cpuid branch November 21, 2024 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding CPUID for AVX10.2 #109302

Adding CPUID for AVX10.2 #109302

khushal1996 commented Oct 28, 2024 •

edited

Loading

dotnet-policy-service bot commented Oct 28, 2024

khushal1996 commented Nov 13, 2024

tannergooding commented Nov 18, 2024

khushal1996 commented Nov 18, 2024

tannergooding commented Nov 19, 2024

khushal1996 commented Nov 19, 2024

tannergooding commented Nov 19, 2024

BruceForstall Nov 20, 2024

tannergooding Nov 20, 2024 •

edited

Loading

khushal1996 Nov 20, 2024 •

edited

Loading

khushal1996 Nov 20, 2024

tannergooding Nov 20, 2024 •

edited

Loading

khushal1996 Nov 20, 2024

BruceForstall Nov 20, 2024

tannergooding Nov 20, 2024

anthonycanino Nov 20, 2024

tannergooding Nov 20, 2024

BruceForstall commented Nov 20, 2024

Adding CPUID for AVX10.2 #109302

Adding CPUID for AVX10.2 #109302

Conversation

khushal1996 commented Oct 28, 2024 • edited Loading

Overview

Testing Correctness of Logic

Testing JIT test suite

Testing ASMDIFFS

dotnet-policy-service bot commented Oct 28, 2024

khushal1996 commented Nov 13, 2024

tannergooding commented Nov 18, 2024

khushal1996 commented Nov 18, 2024

tannergooding commented Nov 19, 2024

khushal1996 commented Nov 19, 2024

tannergooding commented Nov 19, 2024

BruceForstall Nov 20, 2024

Choose a reason for hiding this comment

tannergooding Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

khushal1996 Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

khushal1996 Nov 20, 2024

Choose a reason for hiding this comment

tannergooding Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

khushal1996 Nov 20, 2024

Choose a reason for hiding this comment

BruceForstall Nov 20, 2024

Choose a reason for hiding this comment

tannergooding Nov 20, 2024

Choose a reason for hiding this comment

anthonycanino Nov 20, 2024

Choose a reason for hiding this comment

tannergooding Nov 20, 2024

Choose a reason for hiding this comment

BruceForstall commented Nov 20, 2024

khushal1996 commented Oct 28, 2024 •

edited

Loading

tannergooding Nov 20, 2024 •

edited

Loading

khushal1996 Nov 20, 2024 •

edited

Loading

tannergooding Nov 20, 2024 •

edited

Loading