Updating Unix to save/restore Avx512 state #83784

tannergooding · 2023-03-22T16:28:06Z

This resolves #81846

tannergooding · 2023-03-22T18:51:53Z

This updates the pal CONTEXT struct to track the necessary additional fields and to populate/restore them as appropriate.

It tries to keep the AVX512 state "pay for play", effectively only costing an additional branch on hardware without support.

I opted to not "exactly" mirror the Win32 API surface. That is, I don't expose or use things like LocateXStateFeature on Unix. Instead, I directly appended these to the end of CONTEXT (rather than having them existing "implicitly") and just consume the relevant fields directly where required.

src/coreclr/pal/inc/pal.h

src/coreclr/pal/src/include/pal/context.h

janvorli

LGTM, thank you!

src/coreclr/gc/vxsort/isa_detection.cpp

tannergooding · 2023-03-24T16:15:56Z

Marked NO-MERGE while I try to figure out the OSX "invalid instruction" failure.

I've not been able to repro locally yet and so I'm trying to root cause it in CI at the moment.

tannergooding · 2023-03-24T23:24:46Z

@janvorli, this could use one more small review pass if you could.

I've consolidated the changes since your last review into one additional commit: 9c841a0

The summary is that most of the changes are small fixes and probably don't need "extra scrutiny". However, the logic required to support OSX in particular had to change quite a bit.

In particular the changes are:

The built-in __cpuid on Unix does not explicitly clear ECX. The MSVC intrinsic does, a couple callsites had to be updated to account for this and to use __cpuidex instead so the behavior is consistent on Unix.
The alignment of the Zmm0H and Zmm16 fields needed to be changed as there are several spots relying on CONTEXT to have 16 byte alignment. Refactoring this to support 32 or 64-byte alignment was going to be non-trivial.
MacOS does its AVX-512 enablement differently from everyone else and we had to change the context restore logic as well as the general querying logic to account for this. It is covered in more detail under https://github.com/apple/darwin-xnu/blob/main/osfmk/i386/fpu.c#L174
- Ended up commenting this bit out and changing OSX to return constant false. There is still some edge case where AVX512 enablement isn't working correctly and a general consideration that managed threads likely need to explicitly opt-in to ensure user code doesn't fault on first use

I attempted to ensure the relevant areas have explanatory comments/links and that the overall logic is shared where feasible.

…e resolved

jkotas · 2023-03-25T14:40:39Z

src/coreclr/vm/amd64/unixstubs.cpp

+        //
+        // See https://github.com/apple/darwin-xnu/blob/main/osfmk/i386/fpu.c#L174
+
+        // TODO-AVX512: Enabling this for OSX requires ensuring threads explicitly trigger


To me, it sounds like that it is not worth the troubles to enable AVX512 for OSX x64.

That's certainly possible. I think we do need some comment tracking/explaining the general reason why its not supported however so the only other change I could see is removing the commented out code.

Updated to remove the commented out code. I still left the general TODO, but its likely not something we'll actively focus on supporting given the general move of OSX to Arm64.

OSX may have similar requirements for SVE, however, so if the work eventually happens, it should still be possible to enable in the future.

…explaining

tannergooding · 2023-03-25T19:25:55Z

Finally all green, 🎉 OSX was a lot more problematic than expected 😅

janvorli · 2023-03-27T08:39:22Z

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

@@ -682,8 +755,34 @@ void CONTEXTToNativeContext(CONST CONTEXT *lpContext, native_context_t *native)
 #if defined(HOST_AMD64) && defined(XSTATE_SUPPORTED)
    if ((lpContext->ContextFlags & CONTEXT_XSTATE) == CONTEXT_XSTATE)
    {
-        _ASSERTE(FPREG_HasYmmRegisters(native));
-        memcpy_s(FPREG_Xstate_Ymmh(native), sizeof(M128A) * 16, lpContext->VectorRegister, sizeof(M128A) * 16);
+        if (FPREG_HasYmmRegisters(native))


What would be the case when XSTATE is enabled and YMM registers were not present?

From the current implementation perspective, it shouldn't happen.

From the hardware and correctness perspective, xstate carries a lot of information and the YMM area is considered part of the optional extended state. It's safer and overall better to be consistent in our checks, IMO (particularly when the check is cheap compared to the overall thread suspend/restore logic).

I think if we want this to be performant, then we'd be better off focusing on transitioning the various logic to use the "native" suspend/restore mechanism for a given platform directly.

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

tannergooding · 2023-03-27T17:31:19Z

Logged #83983 to track the additional MacOS save/restore support.

It's possible the reason this was still failing is due to a bad interaction with our own hardware exception handler logic so we'll do some minimal additional investigation to see if that is the case and if AVX-512 can be enabled for MacOS.

@janvorli, anything else needed here or is this good to be merged now?

janvorli

LGTM, thank you!

tannergooding · 2023-03-27T17:42:46Z

Thanks!

dotnet-issue-labeler bot added the area-PAL-coreclr label Mar 22, 2023

ghost assigned tannergooding Mar 22, 2023

tannergooding force-pushed the evex-unix branch 2 times, most recently from 7cb1fb6 to 5f41f8d Compare March 22, 2023 16:43

Updating Unix to save/restore Avx512 state

aa6e6b2

tannergooding force-pushed the evex-unix branch from 5f41f8d to aa6e6b2 Compare March 22, 2023 17:59

tannergooding added 2 commits March 22, 2023 11:19

Ensure compilation works when XSTATE_SUPPORTED isn't defined

a1779a8

Ensure __cpuid isn't implemented on Arm32/Arm64

cf91641

tannergooding force-pushed the evex-unix branch from 8dd3e06 to cf91641 Compare March 22, 2023 18:49

tannergooding added 5 commits March 22, 2023 12:25

Fixing a couple of build failures

c91ce32

Fixing some OSX failures

80ad388

More OSX fixes

c7898c8

More build fixes

bc780f4

AltJit and another OSX build fix

6aec500

janvorli reviewed Mar 23, 2023

View reviewed changes

src/coreclr/pal/inc/pal.h Show resolved Hide resolved

janvorli reviewed Mar 23, 2023

View reviewed changes

src/coreclr/pal/src/include/pal/context.h Outdated Show resolved Hide resolved

src/coreclr/pal/src/include/pal/context.h Outdated Show resolved Hide resolved

tannergooding added 3 commits March 23, 2023 07:20

Ensure an assert checks the right bit

3875cfa

Fixing an assert on OSX to account for general XSTATE tracking

d6664dd

Addressing PR feedback

ac794a3

runfoapp bot mentioned this pull request Mar 23, 2023

Infra improvements for Helix #68176

Closed

janvorli approved these changes Mar 23, 2023

View reviewed changes

tannergooding marked this pull request as ready for review March 23, 2023 15:51

tannergooding requested a review from MichalStrehovsky as a code owner March 23, 2023 15:51

build-analysis bot mentioned this pull request Mar 23, 2023

Arm32: System.Threading.Tasks.Dataflow.Tests failing with NRE #80857

Closed

tannergooding commented Mar 23, 2023

View reviewed changes

src/coreclr/gc/vxsort/isa_detection.cpp Show resolved Hide resolved

tannergooding force-pushed the evex-unix branch 3 times, most recently from b50270e to 7ad3f68 Compare March 24, 2023 18:56

build-analysis bot mentioned this pull request Mar 24, 2023

WasmTestOnBrowser-System.* test failures in CI #83655

Closed

Various fixups and bug fixes to appease CI

9c841a0

tannergooding force-pushed the evex-unix branch from 6b52fac to 9c841a0 Compare March 24, 2023 23:13

tannergooding removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 24, 2023

Use HOST_AMD64 not HOST_64BIT

ac62da6

tannergooding force-pushed the evex-unix branch from f13f332 to ac62da6 Compare March 24, 2023 23:58

Explicitly bypass AVX512 support on OSX until thread enablement can b…

9a84121

…e resolved

jkotas reviewed Mar 25, 2023

View reviewed changes

tannergooding added 3 commits March 25, 2023 08:40

Merge remote-tracking branch 'dotnet/main' into evex-unix

441ecaa

Remove commented out code for OSX in favor of just leaving a comment …

8dab7e5

…explaining

Use HAVE_MACH_EXCEPTIONS not TARGET_OSX

a3bbf0e

BruceForstall added the avx512 Related to the AVX-512 architecture label Mar 27, 2023

janvorli reviewed Mar 27, 2023

View reviewed changes

src/coreclr/pal/src/thread/context.cpp Show resolved Hide resolved

tannergooding mentioned this pull request Mar 27, 2023

MacOS save/restore for AVX-512 state #83983

Open

janvorli approved these changes Mar 27, 2023

View reviewed changes

tannergooding merged commit d612a50 into dotnet:main Mar 27, 2023

tannergooding deleted the evex-unix branch March 27, 2023 17:42

tannergooding mentioned this pull request Mar 28, 2023

JIT: small OSR locals must be normalize on load #84000

Merged

EgorBo mentioned this pull request Mar 28, 2023

[Perf] Alpine/x64: 2 Regressions on 3/27/2023 6:14:52 PM dotnet/perf-autofiling-issues#14505

Open

EgorBo mentioned this pull request Apr 4, 2023

Regressions in Exceptions.Handling #84308

Closed

ghost locked as resolved and limited conversation to collaborators Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating Unix to save/restore Avx512 state #83784

Updating Unix to save/restore Avx512 state #83784

tannergooding commented Mar 22, 2023 •

edited

Loading

tannergooding commented Mar 22, 2023

janvorli left a comment

tannergooding commented Mar 24, 2023

tannergooding commented Mar 24, 2023 •

edited

Loading

jkotas Mar 25, 2023

tannergooding Mar 25, 2023

tannergooding Mar 25, 2023 •

edited

Loading

tannergooding commented Mar 25, 2023

janvorli Mar 27, 2023

tannergooding Mar 27, 2023

tannergooding commented Mar 27, 2023

janvorli left a comment

tannergooding commented Mar 27, 2023

Updating Unix to save/restore Avx512 state #83784

Updating Unix to save/restore Avx512 state #83784

Conversation

tannergooding commented Mar 22, 2023 • edited Loading

tannergooding commented Mar 22, 2023

janvorli left a comment

Choose a reason for hiding this comment

tannergooding commented Mar 24, 2023

tannergooding commented Mar 24, 2023 • edited Loading

jkotas Mar 25, 2023

Choose a reason for hiding this comment

tannergooding Mar 25, 2023

Choose a reason for hiding this comment

tannergooding Mar 25, 2023 • edited Loading

Choose a reason for hiding this comment

tannergooding commented Mar 25, 2023

janvorli Mar 27, 2023

Choose a reason for hiding this comment

tannergooding Mar 27, 2023

Choose a reason for hiding this comment

tannergooding commented Mar 27, 2023

janvorli left a comment

Choose a reason for hiding this comment

tannergooding commented Mar 27, 2023

tannergooding commented Mar 22, 2023 •

edited

Loading

tannergooding commented Mar 24, 2023 •

edited

Loading

tannergooding Mar 25, 2023 •

edited

Loading