Skip to content
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

Add X64/Arm64 nested classes to System.Runtime.Intrinsics where missing #38460

Merged
merged 7 commits into from
Jul 10, 2020

Conversation

tannergooding
Copy link
Member

This resolves #34587 by adding in X64 and Arm64 nested classes to the System.Runtime.Intriniscs classes where they were missing.

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Jun 26, 2020
@tannergooding
Copy link
Member Author

CC. @echesakovMSFT, @CarolEidt for review.

CC. @EgorBo, this seems to have broken Mono, is there something specific I need to update?

@EgorBo
Copy link
Member

EgorBo commented Jul 6, 2020

@tannergooding looks like it's a different issue, so should be OK from mono side.

@tannergooding
Copy link
Member Author

Alright, it looked to be a failure in the test this PR is adding, so I wasn't sure. Thanks!

@tannergooding
Copy link
Member Author

@EgorBo, looks like its still failing. Based on the failure, some API is returning IsSupported where a lower ISA is not. It isn't clear which API that is as there is no line information in the failure.

@EgorBo
Copy link
Member

EgorBo commented Jul 6, 2020

@tannergooding where can I find that test? I don't see any Runtime_34587 test in the sources, is it internal?

@tannergooding
Copy link
Member Author

Right here: https://github.com/dotnet/runtime/pull/38460/files#diff-e4e58d0c6a832040d081a159b95e83bb

It just validates that for any IsSupported == true, the relevant related conditions are met.

@EgorBo
Copy link
Member

EgorBo commented Jul 6, 2020

Right here: https://github.com/dotnet/runtime/pull/38460/files#diff-e4e58d0c6a832040d081a159b95e83bb

It just validates that for any IsSupported == true, the relevant related conditions are met.

can you please add this test to issues.targets to ignore on Mono and I'll take a look tomorrow.
it looks like our hierarchy is a bit outdated e.g. we don't support X86Base

Copy link
Contributor

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM

@jkotas
Copy link
Member

jkotas commented Jul 22, 2020

This was JIT/EE interface breaking change and it should have updated the JIT/EE interface GUID.

I am adding a note about it in #39777

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
@tannergooding tannergooding deleted the fix-34587 branch November 11, 2022 15:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add empty X64/Arm64 nested classes to some System.Runtime.Intrinsics
5 participants