Update the CPUID and XSAVE logics for APX #104637
Conversation
dotnet-issue-labeler
bot
added
the
needs-area-label
dotnet-policy-service
bot
added
the
community-contribution
|
Hi @tannergooding, I reopened the PR for APX CPUID updates here, I will resolve the conflict on guid, let CI start soon and fix fails popping up. I understand this PR is targeting on the next release cycle, say .Net 10, so when your schedule allows, I wonder if you could review this PR for the first round, thanks! |
|
resolved the conflict. |
am11
added
area-PAL-coreclr
and removed
needs-area-label
Ruihan-Yin
changed the title
|
The REX2 enabling PR (#106557) is there, that PR is based on this one, since now main is accepting .net 10 changes, I will rebase the changes to latest main and continue the works. |
Ruihan-Yin
force-pushed
the
apx-CPUID-XSAVE
branch
from
d549714
to
3446e28
Compare
|
resolved conflicts with main |
Ruihan-Yin
changed the title
|
@tannergooding This PR is ready for review. Build failures are related to the changes in https://github.com/dotnet/runtime/blob/main/src/coreclr/pal/src/arch/amd64/context2.S the native compiler seems to not recognize EGPRs, e.g. r16 as a legal operand. I'm not sure how we can resolve this or if this part is needed for now. And for the changes to accommodate the XSTATE changes for EGPRs, I would be very willing to taking some suggestions from the high level, there might be some changes missing or not reasonable at all. it will be much appreciated if some advice can be shared. |
There was a problem hiding this comment.
We will also need to update the CONTEXT& CONTEXT::operator=(const CONTEXT& ctx) to properly copy the new registers if the XStateFeaturesMask has a mask for this feature. And also make sure that when it is not set, we copy only the necessary part of the context.
| @@ -183,6 +183,29 @@ LOCAL_LABEL(Done_Restore_CONTEXT_FLOATING_POINT): | |||
| kmovq k6, qword ptr [rdi + (CONTEXT_KMask0 + 6 * 8)] | |||
| kmovq k7, qword ptr [rdi + (CONTEXT_KMask0 + 7 * 8)] | |||
|
|
|||
| // TODO-xarch-apx: the definition of XSTATE mask value for APX is now missing on the OS level, | |||
There was a problem hiding this comment.
This is Unix only code, so the XSTATE mask is not coming from the OS headers, but rather pal.h for C/C++ code and src/coreclr/pal/src/arch/amd64/asmconstants.h for asm. This comment can be removed.
| // TODO-xarch-apx: the definition of XSTATE mask value for APX is now missing on the OS level, | ||
| // we are currently using bare value to hack it through the build process, and test the implementation through CI. | ||
| // those changes will be removed when we have the OS support for APX. | ||
| test BYTE PTR [rdi + CONTEXT_XStateFeaturesMask], 524288 |
There was a problem hiding this comment.
Can you please add symbolic definition for the mask to src/coreclr/pal/src/arch/amd64/asmconstants.h next to the other XSTATE_xxx definitions?
| @@ -3069,7 +3079,7 @@ BOOL Thread::RedirectCurrentThreadAtHandledJITCase(PFN_REDIRECTTARGET pTgt, CONT | |||
| if (srcFeatures != 0) | |||
| { | |||
| #if defined(TARGET_X86) || defined(TARGET_AMD64) | |||
| const DWORD64 xStateFeatureMask = XSTATE_MASK_AVX | XSTATE_MASK_AVX512; | |||
| const DWORD64 xStateFeatureMask = XSTATE_MASK_AVX | XSTATE_MASK_AVX512 | XSTATE_MASK_APX; | |||
There was a problem hiding this comment.
This is Windows only code. I think that SetXStateFeaturesMask will fail if we pass in an unknown mask.
There was a problem hiding this comment.
It explicitly allows unknown masks, so that you don't need to do version/build specific checks (https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setxstatefeaturesmask):
The system silently ignores any feature specified in the FeatureMask which is not enabled on the processor.
| public const int SIZEOF__REGDISPLAY = 0x1a90; | ||
| public const int OFFSETOF__REGDISPLAY__SP = 0x1a78; | ||
| public const int OFFSETOF__REGDISPLAY__ControlPC = 0x1a80; | ||
| public const int SIZEOF__REGDISPLAY = 0x1b90; |
There was a problem hiding this comment.
I'm not sure how these values are calculated, the values are updated based on the error massage, so want to double check with the reviewers.
There was a problem hiding this comment.
These constants are what the sizeof(...) / offsetof(...) would get during C/C++ compilation. The offsets above are verified to match with the real values during the C/C++ compilation phase, so if the runtime build passes, you are safe.
There was a problem hiding this comment.
Thanks for the explanation!
|
-- This is still on my backlog to review, just dealing with some remaining .NET 9 RC2 items before I can get to it. |
BruceForstall
added
the
apx
Previously #103019
Overview on the changes:
XArchIntrinsicConstants: Compress all the Avx512 related flags into 1 -Avx512f+bw+cd+dq+vltoAvx512, this saves more space inXArchIntrinsicConstantsso that we can hold more x86 ISAs, like here, APX.CR4[XSAVE](existing) ->XCR0[APX_F]->CPUID(7,1).EDX[APX_F]- the current status is that due to the missing macro definition for APX on the OS level, the second check will fail anyways, and it may break the build on CI (to be verified).