Skip to content

Conversation

Ruihan-Yin
Copy link
Member

@Ruihan-Yin Ruihan-Yin commented Apr 15, 2025

resolving the comments from #104637.

renamed Egprx to Rx.

Fixed the copy logic for CONTEXT data structure, now it should be able to correctly handle the cases with Avx512 and APX.

@ghost ghost added the area-PAL-coreclr label Apr 15, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 15, 2025
@Ruihan-Yin
Copy link
Member Author

Hi @BruceForstall, I made the changes to revert the raw byte code in contetx2.S. Looks like some builds are still failing: OSX/tvOS, linux with GCC and musl.

Are these in this location because it mimics the layout of XSAVE? Or is it arbitrary? Why are these named "Egpr16" instead of just "R16"?

Also regarding this question, I have renamed the registers, and the layout should be arbitrary, we use __cpuid(0D, featureIndex) to query the size and offset of the XSTATE feature, and then load/store them into a abstracted data structure context. Detail behavior is defined in CONTEXTToNativeContext in context.cpp. So I think we don't need to change the layout.

But I do have another open to discuss, I've logged the details in context.cpp. It would be much appreciated if you could share some thoughts.

@BruceForstall
Copy link
Contributor

I presume we will never support APX on OSX/tvOS or any Apple product, since they are actively deprecating Intel processor support. So the build should work around that with FEATURE_APX ifdefs or similar. If GCC and musl don't support an updated assembler yet, then I guess the change to symbolic code can't be made.

But I do have another open to discuss, I've logged the details in context.cpp. It would be much appreciated if you could share some thoughts.

This is a question about CONTEXT handling in the PAL -- @janvorli or @jkotas might be better to answer.

@janvorli
Copy link
Member

@Ruihan-Yin there is no need to copy the contents of the CONTEXT in one block. You can copy the APX part separately. It was just a convenience when there was one extra block, so we could do a single copy.

@Ruihan-Yin
Copy link
Member Author

@Ruihan-Yin there is no need to copy the contents of the CONTEXT in one block. You can copy the APX part separately. It was just a convenience when there was one extra block, so we could do a single copy.

Thanks for the clarification, I will make the changes accordingly.

@Ruihan-Yin
Copy link
Member Author

I presume we will never support APX on OSX/tvOS or any Apple product, since they are actively deprecating Intel processor support. So the build should work around that with FEATURE_APX ifdefs or similar. If GCC and musl don't support an updated assembler yet, then I guess the change to symbolic code can't be made.

But I do have another open to discuss, I've logged the details in context.cpp. It would be much appreciated if you could share some thoughts.

This is a question about CONTEXT handling in the PAL -- @janvorli or @jkotas might be better to answer.

Now that we still have dependency on GCC and musl, we can let the raw byte code stay for a little longer. But I can still cover the needed changes for renaming and context copy in this PR

@Ruihan-Yin Ruihan-Yin closed this May 5, 2025
@Ruihan-Yin Ruihan-Yin reopened this May 5, 2025
@Ruihan-Yin Ruihan-Yin changed the title [APX] test some experiment changes on CI [APX] Follow up on #104637: rename extended registers and bug fixes. May 9, 2025
@Ruihan-Yin Ruihan-Yin marked this pull request as ready for review May 9, 2025 16:23
@Copilot Copilot AI review requested due to automatic review settings May 9, 2025 16:23
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR renames extended registers from Egprx to Rx and fixes the copy logic for the CONTEXT structure related to AVX512 and APX handling.

  • Renamed register fields from Egpr16–Egpr31 to R16–R31 in the PAL header.
  • Updated memcpy calls in context.cpp to use the new register naming.
  • Adjusted the assignment operator to separate APX register copying while handling AVX512.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/coreclr/pal/src/thread/context.cpp Updated memcpy calls and assignment operator logic to reflect register renaming and fix copy issues.
src/coreclr/pal/inc/pal.h Renamed structure members from Egpr16–Egpr31 to R16–R31.
Comments suppressed due to low confidence (1)

src/coreclr/pal/inc/pal.h:1523

  • [nitpick] The register fields have been renamed to 'R16-R31'; please verify that all related code references are updated accordingly.
DWORD64 R16;

@Ruihan-Yin
Copy link
Member Author

failures look irrelevant, rerunning CI
@dotnet/intel for review

make sure we APX registers are copied to the right memory location.

Co-authored-by: Copilot <[email protected]>
@Ruihan-Yin
Copy link
Member Author

Hi @BruceForstall, could you please give this another review? Thanks!

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. @janvorli or @jkotas should also sign-off since this is a PAL side change

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@tannergooding tannergooding merged commit ce6d6d4 into dotnet:main May 15, 2025
95 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-PAL-coreclr 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.

4 participants