Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding zmmStateSupport and AVX512F, AVX512CD, AVX512BW, AVX512DQ and AVX512VL ISAs. #74113

Merged
merged 3 commits into from
Sep 16, 2022

Conversation

DeepakRajendrakumaran
Copy link
Contributor

This change adds logic for detecting avx512 support via zmmStateSupport and the logic in EEJitManager::SetCpuInfo()

For specifics of zmmStateSupport- see section 15.2 DETECTION OF AVX-512 FOUNDATION INSTRUCTIONS in Intel® 64 and IA-32 Architectures Software Developer’s Manual Combined Volumes: 1, 2A, 2B, 2C, 2D, 3A, 3B, 3C, 3D, and 4 for details. 14.3 DETECTION OF AVX INSTRUCTIONS details logic used for AVX and SSE.

The other changes are to add new AVX512 ISAs. Most of these changes are autogenerated code via src/coreclr/tools/Common/JitInterface/ThunkGenerator/InstructionSetDesc.txt

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 18, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@DeepakRajendrakumaran
Copy link
Contributor Author

@DeepakRajendrakumaran
Copy link
Contributor Author

@tannergooding I cannot figure out where/how XmmYmmStateSupport in src/coreclr/pal/src/arch/amd64/processor.cpp is used. And why it's XmmYmmStateSupport as opposed to xmmYmmStateSupport . Are you familiar with this call?

@tannergooding
Copy link
Member

I cannot figure out where/how XmmYmmStateSupport in src/coreclr/pal/src/arch/amd64/processor.cpp is used. And why it's XmmYmmStateSupport as opposed to xmmYmmStateSupport . Are you familiar with this call?

I don't see any existing consumers, it might be dead code.

@tannergooding
Copy link
Member

There's still a couple of pending comments above and we need to revise the JIT/EE Version Guid here: https://github.com/dotnet/runtime/blob/main/src/coreclr/inc/jiteeversionguid.h

Otherwise the changes look good/correct to me and I think we can adjust the _VL handling if needed later. Would still be good if someone from @dotnet/crossgen-contrib could review those changes as well, however.

@DeepakRajendrakumaran
Copy link
Contributor Author

There's still a couple of pending comments above and we need to revise the JIT/EE Version Guid here: https://github.com/dotnet/runtime/blob/main/src/coreclr/inc/jiteeversionguid.h

Otherwise the changes look good/correct to me and I think we can adjust the _VL handling if needed later. Would still be good if someone from @dotnet/crossgen-contrib could review those changes as well, however.

Have added the requested changes.

@BruceForstall
Copy link
Member

Tagging the group... just to see it work... @dotnet/avx512-contrib

@DeepakRajendrakumaran
Copy link
Contributor Author

Tagging the group... just to see it work... @dotnet/avx512-contrib

Works for me. Got an email notification

@DeepakRajendrakumaran
Copy link
Contributor Author

There's still a couple of pending comments above and we need to revise the JIT/EE Version Guid here: https://github.com/dotnet/runtime/blob/main/src/coreclr/inc/jiteeversionguid.h

Otherwise the changes look good/correct to me and I think we can adjust the _VL handling if needed later. Would still be good if someone from @dotnet/crossgen-contrib could review those changes as well, however.

@tannergooding I had originally missed your comment re JIT/EE Version Guid. I have updated it now following instruction in the file(ran "uuidgen.exe -s" locally and replaced the GUID in file with that). Please let me know if there is anything else needed from us to push this forward.

@davidwrighton
Copy link
Member

I've approved, but I'd like to see an approval from @tannergooding as well before we merge.

Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minus the question about the NAOT instruction set names

@@ -19,3 +19,20 @@ LEAF_ENTRY xmmYmmStateSupport, _TEXT
LEAF_END xmmYmmStateSupport, _TEXT

end

;; extern "C" DWORD __stdcall avx512StateSupport();
LEAF_ENTRY avx512StateSupport, _TEXT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be hooked up here:

bool DetectCPUFeatures()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the AVX512 ISA checks need to be mirrored there as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the 'end' was in the wrong place :) . Have added the checks in startup.cpp

@tannergooding
Copy link
Member

Failures are #75667

super-pmi failures are due to the JIT/EE version guid update

timeouts are on a subset of arm/arm64 machines and are happening everywhere, this change is explicitly around xarch.

@tannergooding tannergooding merged commit 242c95a into dotnet:main Sep 16, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 17, 2022
@BruceForstall BruceForstall added the avx512 Related to the AVX-512 architecture label Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-coreclr avx512 Related to the AVX-512 architecture community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants