Update the CPUID and XSAVE logics for APX #104637

Ruihan-Yin · 2024-07-09T20:03:26Z

Previously #103019

Overview on the changes:

Infrastructural changes on XArchIntrinsicConstants: Compress all the Avx512 related flags into 1 - Avx512f+bw+cd+dq+vl to Avx512, this saves more space in XArchIntrinsicConstants so that we can hold more x86 ISAs, like here, APX.

Edit: remove unnecessary Avx512VL ISA flags. #103144 and Cleanup some handling around Avx10v1 #103241 have resolved this part.

CPUID check updates for 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).

Edit: made some workaround for this part in the code.

APX introduced 16 EGPRs, which are XSTATE enabled, this PR provides the corresponding changes for XSAVE behaviors.

Ruihan-Yin · 2024-07-16T00:05:47Z

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!

Ruihan-Yin · 2024-07-17T23:05:52Z

resolved the conflict.

Ruihan-Yin · 2024-08-16T21:41:34Z

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 · 2024-08-28T21:55:26Z

resolved conflicts with main

Ruihan-Yin · 2024-08-29T20:33:34Z

@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.

janvorli

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.

janvorli · 2024-08-29T22:52:12Z

src/coreclr/pal/src/arch/amd64/context2.S

@@ -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, 


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.

janvorli · 2024-08-29T22:55:33Z

src/coreclr/pal/src/arch/amd64/context2.S

+    // 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


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?

src/coreclr/vm/threadsuspend.cpp

BruceForstall · 2024-11-04T23:54:35Z

linux and arm related build failures originate from the ASM code that uses EGPRs in the MOV instructions, while now the compiler (presumably LLVM) does not accept EGPRs as operand. This might be able to be fixed by updating the dependent version of LLVM.

Did you fix this issue?

We're using clang 18.1.8 to build (in azure Linux containers): dotnet/dotnet-buildtools-prereqs-docker#1207). What version has the APX features required?

BruceForstall · 2024-11-05T00:06:07Z

src/coreclr/vm/threadsuspend.cpp

@@ -71,6 +71,16 @@ extern "C" void             RedirectedHandledJITCaseForGCStress_Stub(void);
 #define IS_VALID_WRITE_PTR(addr, size)      _ASSERTE((addr) != NULL)
 #define IS_VALID_CODE_PTR(addr)             _ASSERTE((addr) != NULL)

+#if defined(TARGET_AMD64) || defined(TARGET_X86)
+// These values should be picked up from winrt.h, defining them in case they are missing there.


nit

Suggested change

// These values should be picked up from winrt.h, defining them in case they are missing there.

// These values should be picked up from winnt.h, defining them in case they are missing there.

BruceForstall · 2024-11-05T00:07:29Z

src/native/minipal/cpufeatures.c

+static uint32_t apxStateSupport()
+{
+#if defined(HOST_APPLE)
+    return false;


Shouldn't this be zero, or an assert, and not false?

jkotas · 2024-11-05T00:13:37Z

docs/design/features/xarch-apx.md

@@ -0,0 +1,3 @@
+# APX Integration in .NET


This doc can be added only once there is something worth talking about.

Sure, I can remove this for now and may add some specification to the design when REX2 or APX-EVEX changes coming in.

Ruihan-Yin · 2024-11-07T18:30:47Z

linux and arm related build failures originate from the ASM code that uses EGPRs in the MOV instructions, while now the compiler (presumably LLVM) does not accept EGPRs as operand. This might be able to be fixed by updating the dependent version of LLVM.

Did you fix this issue?

We're using clang 18.1.8 to build (in azure Linux containers): dotnet/dotnet-buildtools-prereqs-docker#1207). What version has the APX features required?

We've communicated with our LLVM folks and were told LLVM-19 should have APX support, but eventually we will need LLVM-20 to accommodate the Avx10.2 codegen changes.

Do you have any preference how we can make this through? we are okay with upgrading to LLVM-19 this time and LLVM-20 later, or upgrade once altogether. (LLVM-20 release should be out early next year.)

src/coreclr/pal/src/arch/amd64/context2.S

janvorli · 2024-11-08T19:54:53Z

src/coreclr/pal/src/arch/amd64/asmconstants.h

@@ -91,7 +93,8 @@
 #define CONTEXT_KMask0 CONTEXT_Ymm0H+(16*16)
 #define CONTEXT_Zmm0H CONTEXT_KMask0+(8*8)
 #define CONTEXT_Zmm16 CONTEXT_Zmm0H+(32*16)
-#define CONTEXT_Size CONTEXT_Zmm16+(64*16)
+#define CONTEXT_Egpr  CONTEXT_Zmm16+(16*8)
+#define CONTEXT_Size CONTEXT_Egpr+(64*16)


This doesn't look right. It seems it should be

#define CONTEXT_Egpr CONTEXT_Zmm16+(64*16) #define CONTEXT_Size CONTEXT_Egpr+(16*8)

Looks like this comment is still pending.

I'd agree with the janvorli's assessment here. Each new define is previousDefine+(SizeOfPreviousDefine) and _Zmm16 should have a size of 64*16, while _Egpr should have a size of 8*16 (typically SizeOfPreviousDefine is built up as SizePerItem*NumberOfItems, so 64*16 because each register is 64-bytes and there are 16 of them, similarly 8*16 because each register is 8-bytes and there are 16 of them).

Thanks for pointing out and the explanation!

sorry for the delay, we had a holiday yesterday.

janvorli · 2024-11-08T19:58:33Z

src/coreclr/tools/Common/JitInterface/ThunkGenerator/InstructionSetDesc.txt

@@ -22,6 +22,8 @@

 ; DO NOT CHANGE R2R NUMERIC VALUES OF THE EXISTING SETS. Changing R2R numeric values definitions would be R2R format breaking change.

+; The ISA definiitons should also be mapped to `hwintrinsicIsaRangeArray` in hwintrinsic.cpp.


A nit:

Suggested change

; The ISA definiitons should also be mapped to `hwintrinsicIsaRangeArray` in hwintrinsic.cpp.

; The ISA definitions should also be mapped to `hwintrinsicIsaRangeArray` in hwintrinsic.cpp.

janvorli · 2024-11-08T20:01:28Z

src/native/minipal/cpufeatures.c

@@ -125,6 +155,19 @@ static uint32_t avx512StateSupport()
    return ((_xgetbv(0) & 0xE6) == 0x0E6) ? 1 : 0;
 }

+#ifndef XSTATE_MASK_APX


This was already defined in this file at line 75

Line 75 is the definition under UNIX (Avx512 seems to adopt the design that have these mask only explicitly defined under UNIX, so I followed it for APX). And under windows, I suppose the definition should be from winnt.h, I'm not quite familiar with how PAL works with each part. If so, we will need this definition here until OS supports APX and defines these masks in winnt?

Ah, I've missed the fact that one definition was for Windows and the other for Unix. So we can keep it as is until the symbol makes it to the windows SDK and then remove the Windows defines.

janvorli · 2024-11-08T20:03:35Z

src/coreclr/pal/src/thread/context.cpp

@@ -313,6 +313,16 @@ typedef int __ptrace_request;
        ASSIGN_CONTROL_REGS \
        ASSIGN_INTEGER_REGS \

+#if defined(HOST_AMD64)  && defined(XSTATE_SUPPORTED)


It was already defined in pal.h, it should be visible here.

tannergooding · 2024-11-11T18:29:43Z

src/coreclr/pal/src/thread/context.cpp

@@ -809,6 +872,18 @@ void CONTEXTToNativeContext(CONST CONTEXT *lpContext, native_context_t *native)
                dest = FPREG_Xstate_Hi16Zmm(native, &size);
                _ASSERT(size == (sizeof(M512) * 16));
                memcpy_s(dest, sizeof(M512) * 16, &lpContext->Zmm16, sizeof(M512) * 16);
+
+#ifndef TARGET_OSX
+                // TODO-xarch-apx: I suppose OSX will not support APX.


This might be dependent on whether or not Rosetta gets support, correct?

My personal educated guess is that Rosetta will never get support for APX (like it never gets support for AVX). The goal of Rosetta is to run apps developed for intel based macOS devices. Since those are not being created anymore, emulating APX doesn't seem to make sense.

it never gets support for AVX

I've seen some unofficial reports that they added it in macOS 15.

tannergooding · 2024-11-11T18:33:43Z

src/coreclr/vm/amd64/asmconstants.h

@@ -238,8 +238,8 @@ ASMCONSTANTS_C_ASSERT(OFFSETOF__VASigCookie__pNDirectILStub

 #if defined(UNIX_AMD64_ABI) && !defined(HOST_WINDOWS)
 // Expression is too complicated, is currently:
-//     (8*6 + 4*2 + 2*6 + 4 + 8*6 + 8*16 + 8 + /*XMM_SAVE_AREA32*/(2*2 + 1*2 + 2 + 4 + 2*2 + 4 + 2*2 + 4*2 + 16*8 + 16*16 + 1*96) + 26*16 + 8 + 8*5 + /*XSTATE*/ + 8 + 8 + /*XSTATE_AVX*/ 16*16 + /*XSTATE_AVX512_KMASK*/ 8*8 + /*XSTATE_AVX512_ZMM_H*/ 32*16 + /*XSTATE_AVX512_ZMM*/ 64*16)
-#define               SIZEOF__CONTEXT                 (3104)
+//     (8*6 + 4*2 + 2*6 + 4 + 8*6 + 8*16 + 8 + /*XMM_SAVE_AREA32*/(2*2 + 1*2 + 2 + 4 + 2*2 + 4 + 2*2 + 4*2 + 16*8 + 16*16 + 1*96) + 26*16 + 8 + 8*5 + /*XSTATE*/ + 8 + 8 + /*XSTATE_AVX*/ 16*16 + /*XSTATE_AVX512_KMASK*/ 8*8 + /*XSTATE_AVX512_ZMM_H*/ 32*16 + /*XSTATE_AVX512_ZMM*/ 64*16 + /*XSTATE_APX*/ 8*16)


This comment is getting fairly long. I wonder if we can separate it by line instead to make it more readable, something like this (assuming I broke it up correctly):

// 8*6 + 4*2 + 2*6 + 4 + 8*6 + 8*16 + 8 + // /*XMM_SAVE_AREA32*/ (2*2 + 1*2 + 2 + 4 + 2*2 + 4 + 2*2 + 4*2 + 16*8 + 16*16 + 1*96) + 26*16 + 8 + 8*5 + // /*XSTATE*/ 8 + 8 + // /*XSTATE_AVX*/ 16*16 + // /*XSTATE_AVX512_KMASK*/ 8*8 + // /*XSTATE_AVX512_ZMM_H*/ 32*16 + // /*XSTATE_AVX512_ZMM*/ 64*16 + // /*XSTATE_APX*/ 8*16

tannergooding · 2024-11-11T18:34:43Z

Changes look overall good/correct to me. There's a couple pending cleanup requests from janvorli that should be resolved before sign-off happens

Ruihan-Yin · 2024-11-12T20:10:26Z

CI fails look known or unrelated.

janvorli

LGTM, thank you!

janvorli · 2024-11-12T20:37:19Z

Note: we may get some perf regressions in the performance microbenchmarks due to the larger context struct size.

tannergooding · 2024-11-13T17:24:21Z

Updated the branch to retrigger CI and see if the unrelated Mono WASM issues are resolved.

If not, I'll get issues opened and get this merged once CI finishes again.

DeepakRajendrakumaran · 2024-11-13T17:33:35Z

Updated the branch to retrigger CI and see if the unrelated Mono WASM issues are resolved.

If not, I'll get issues opened and get this merged once CI finishes again.

Thanks Tanner. Looking forward to having this in. Would make running superpmi for APX way easier :)

MichalPetryka · 2024-11-13T17:59:03Z

src/coreclr/pal/src/arch/amd64/context2.S

+    // TODO-XArch-APX:
+    // we are using raw hex code here to emit EGPRs-related changes,
+    // we will need to come back and re-write this part when assembler supports EGPRs.


Is there some way to do an #if to only use the raw hex on assemblers without support today?

I'm not aware of, do we have sth like LLVM version identifier defined in our code base?

I think this chunk is meant to be converted to MOV instructions once we have LLVM with official APX supports, and the code may not have actual use cases until we have machines with APX.

@tannergooding, could you share some thoughts on this?

I think its fine as is, I don't think its worth supporting code that we can't reliably test and maintain in CI at this point in time.

Rather, lets just have the one path and update when the toolchain can finally be updated to LLVM 19/20 or later

I agree with this too.

JulieLeeMSFT · 2024-11-14T22:05:20Z

@BruceForstall, please review this PR. It’s ready to merge.

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 9, 2024

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

am11 added area-PAL-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 19, 2024

Ruihan-Yin changed the title ~~[NO REVIEW] Update the CPUID and XSAVE logics for APX~~ [NO MERGE] Update the CPUID and XSAVE logics for APX Jul 19, 2024

Ruihan-Yin mentioned this pull request Aug 16, 2024

[JIT] Enable EGPRs in JIT by adding REX2 encoding to the backend. #106557

Open

Ruihan-Yin added 5 commits August 28, 2024 13:41

Add APX doc.

9f8f67c

script-gen changes.

a68865c

XSTATE changes

6dfb92b

hand-written CPUID check part

b998cdf

fix

3446e28

Ruihan-Yin force-pushed the apx-CPUID-XSAVE branch from d549714 to 3446e28 Compare August 28, 2024 21:54

Fix merge error.

9bc008a

Ruihan-Yin changed the title ~~[NO MERGE] Update the CPUID and XSAVE logics for APX~~ Update the CPUID and XSAVE logics for APX Aug 28, 2024

bug fixes

44163d7

build-analysis bot mentioned this pull request Aug 29, 2024

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

Open

3 tasks

Ruihan-Yin added 2 commits August 29, 2024 10:32

Bug fix

600abd0

bug fix

81b47c6

Ruihan-Yin marked this pull request as ready for review August 29, 2024 20:33

Ruihan-Yin requested a review from MichalStrehovsky as a code owner August 29, 2024 20:33

janvorli reviewed Aug 29, 2024

View reviewed changes

resolve commnets.

d516feb

BruceForstall reviewed Nov 5, 2024

View reviewed changes

jkotas reviewed Nov 5, 2024

View reviewed changes

tannergooding reviewed Nov 7, 2024

View reviewed changes

src/coreclr/pal/src/arch/amd64/context2.S Outdated Show resolved Hide resolved

Ruihan-Yin added 2 commits November 8, 2024 10:21

resolve comments

8e1ba4a

use byte code for EGPR XSAVE logics.

ea8949f

janvorli reviewed Nov 8, 2024

View reviewed changes

tannergooding reviewed Nov 11, 2024

View reviewed changes

resolve comments.

597e797

tannergooding approved these changes Nov 12, 2024

View reviewed changes

janvorli approved these changes Nov 12, 2024

View reviewed changes

Merge branch 'main' into apx-CPUID-XSAVE

117c401

MichalPetryka reviewed Nov 13, 2024

View reviewed changes

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

chrome-DebuggerTests failing on mono while waiting for pause #109795

Open

Merge remote-tracking branch 'origin/main' into apx-CPUID-XSAVE

8cc7e8e

This was referenced Nov 14, 2024

[browser][MT] System.Net.Http.Functional.Tests failure ("Why are we setting the target on an unoccupied slot?") #104524

Open

System.Runtime.Tests.WorkItemExecution "crash" on tvOS #106319

Open

	// These values should be picked up from winrt.h, defining them in case they are missing there.
	// These values should be picked up from winnt.h, defining them in case they are missing there.

		@@ -22,6 +22,8 @@

		; DO NOT CHANGE R2R NUMERIC VALUES OF THE EXISTING SETS. Changing R2R numeric values definitions would be R2R format breaking change.

		; The ISA definiitons should also be mapped to `hwintrinsicIsaRangeArray` in hwintrinsic.cpp.

	; The ISA definiitons should also be mapped to `hwintrinsicIsaRangeArray` in hwintrinsic.cpp.
	; The ISA definitions should also be mapped to `hwintrinsicIsaRangeArray` in hwintrinsic.cpp.

Update the CPUID and XSAVE logics for APX #104637

Are you sure you want to change the base?

Update the CPUID and XSAVE logics for APX #104637

Conversation

Ruihan-Yin commented Jul 9, 2024 • edited Loading

Ruihan-Yin commented Jul 16, 2024

Ruihan-Yin commented Jul 17, 2024

Ruihan-Yin commented Aug 16, 2024

Ruihan-Yin commented Aug 28, 2024

Ruihan-Yin commented Aug 29, 2024

janvorli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ruihan-Yin commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ruihan-Yin Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding commented Nov 11, 2024

Ruihan-Yin commented Nov 12, 2024

janvorli left a comment

Choose a reason for hiding this comment

janvorli commented Nov 12, 2024

tannergooding commented Nov 13, 2024

DeepakRajendrakumaran commented Nov 13, 2024

Choose a reason for hiding this comment

Ruihan-Yin Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulieLeeMSFT commented Nov 14, 2024

Ruihan-Yin commented Jul 9, 2024 •

edited

Loading

Ruihan-Yin Nov 8, 2024 •

edited

Loading

Ruihan-Yin Nov 13, 2024 •

edited

Loading