-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Update the CPUID and XSAVE logics for APX #104637
base: main
Are you sure you want to change the base?
Conversation
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. |
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. |
d549714
to
3446e28
Compare
resolved conflicts with main |
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
@tannergooding assume you plan to review this change? |
Previously #103019
Overview on the changes:
XArchIntrinsicConstants
: Compress all the Avx512 related flags into 1 -Avx512f+bw+cd+dq+vl
toAvx512
, this saves more space inXArchIntrinsicConstants
so 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).