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 Sve.IsSupported support #97814

Merged
merged 9 commits into from
Feb 7, 2024
Merged

Conversation

a74nh
Copy link
Contributor

@a74nh a74nh commented Feb 1, 2024

Add the SVE API, but it only includes the IsSupported method.

Add the blank test files to go along with it

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 1, 2024
Copy link

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

@ghost
Copy link

ghost commented Feb 1, 2024

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics
See info in area-owners.md if you want to be subscribed.

Issue Details

Add the SVE API, but it only includes the IsSupported method.

Add the blank test files to go along with it

Author: a74nh
Assignees: -
Labels:

area-System.Runtime.Intrinsics, new-api-needs-documentation, community-contribution

Milestone: -

@a74nh
Copy link
Contributor Author

a74nh commented Feb 1, 2024

This is cut from #97695

I'm still a little confused about the testing. I can get the .cs files to generate. But then I'm not sure how to build and run the generated test files.

Also, what do I need to build+run to get PrintSupportedIsa() (in Shared/Program.cs) to print?

For now, the new SVE block inside GenerateHWIntrinsicTests_Arm.cs just contains a duplication of the existing LoadUnOpTest.template, which can be removed as soon as the first Sve API entry is added.

@kunalspathak @dotnet/arm64-contrib @tannergooding

@a74nh a74nh marked this pull request as ready for review February 1, 2024 14:38
@tannergooding
Copy link
Member

@ryujit-bot
Copy link

Diff results for #97814

Throughput diffs

Throughput diffs for windows/arm64 ran on windows/x64

MinOpts (-0.01% to +0.00%)
Collection PDIFF
libraries.pmi.windows.arm64.checked.mch -0.01%

Details here


Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

@tannergooding - are there other places where we should set the bits for IsSupported or this should be good enough?

@ryujit-bot
Copy link

Diff results for #97814

Throughput diffs

Throughput diffs for linux/arm64 ran on windows/x64

MinOpts (-0.01% to +0.00%)
Collection PDIFF
libraries.pmi.linux.arm64.checked.mch -0.01%

Throughput diffs for windows/arm64 ran on windows/x64

MinOpts (-0.00% to +0.01%)
Collection PDIFF
libraries.pmi.windows.arm64.checked.mch +0.01%

Details here


@a74nh
Copy link
Contributor Author

a74nh commented Feb 2, 2024

Test results on SVE enabled machine running HardwareIntrinsics_Arm_r.sh:

14:29:58.994 Running test: JIT/HardwareIntrinsics/Arm/Sve/Sve_r/Sve_r.dll
Supported ISAs:
  AdvSimd:   True
  Aes:       True
  ArmBase:   True
  Crc32:     True
  Dp:        True
  Rdm:       True
  Sha1:      True
  Sha256:    True
  Sve:       True

Beginning scenario: RunBasicScenario_Load
Beginning scenario: RunReflectionScenario_Load
14:29:59.005 Passed test: JIT/HardwareIntrinsics/Arm/Sve/Sve_r/Sve_r.dll

@a74nh
Copy link
Contributor Author

a74nh commented Feb 5, 2024

2024-02-02T12:15:09.0048116Z ##[error].packages/microsoft.dotnet.apicompat.task/9.0.100-alpha.1.24072.3/build/Microsoft.DotNet.ApiCompat.ValidateAssemblies.Common.targets(16,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) API breaking changes found. If those are intentional, the APICompat suppression file can be updated by rebuilding with '/p:ApiCompatGenerateSuppressionFile=true'

I'm not sure how to fix this in the PR.

@kunalspathak
Copy link
Member

I'm not sure how to fix this in the PR.

there were few missing places. my changes should fix it.

@ryujit-bot
Copy link

Diff results for #97814

Throughput diffs

Throughput diffs for linux/arm64 ran on windows/x64

MinOpts (-0.00% to +0.01%)
Collection PDIFF
libraries.pmi.linux.arm64.checked.mch +0.01%

Details here


@kunalspathak
Copy link
Member

Test results on SVE enabled machine running HardwareIntrinsics_Arm_r.sh:

14:29:58.994 Running test: JIT/HardwareIntrinsics/Arm/Sve/Sve_r/Sve_r.dll
Supported ISAs:
  AdvSimd:   True
  Aes:       True
  ArmBase:   True
  Crc32:     True
  Dp:        True
  Rdm:       True
  Sha1:      True
  Sha256:    True
  Sve:       True

Beginning scenario: RunBasicScenario_Load
Beginning scenario: RunReflectionScenario_Load
14:29:59.005 Passed test: JIT/HardwareIntrinsics/Arm/Sve/Sve_r/Sve_r.dll

@fanyang-mono - how can we verify if this returns false for Mono?

@fanyang-mono
Copy link
Member

fanyang-mono commented Feb 5, 2024

@kunalspathak The change I suggested here is needed, in order for Mono to return false for IsSupported check.

Once this change is in place, you could build mono and write a small program to call IsSupported on Sve to check the value. Note that the CI lane to exercise runtime tests on arm64 is currently disable. Otherwise, we could check CI log as well.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@ryujit-bot
Copy link

Diff results for #97814

Throughput diffs

Throughput diffs for linux/arm64 ran on windows/x64

MinOpts (-0.01% to +0.00%)
Collection PDIFF
libraries.pmi.linux.arm64.checked.mch -0.01%

Details here


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.

The changes in general LGTM, but the Sve.PlatformNotSupported.cs file in particular needs to follow the correct pattern.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 6, 2024
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 7, 2024
@ryujit-bot
Copy link

Diff results for #97814

Throughput diffs

Throughput diffs for linux/arm64 ran on windows/x64

MinOpts (-0.01% to +0.00%)
Collection PDIFF
libraries.pmi.linux.arm64.checked.mch -0.01%

Throughput diffs for windows/arm64 ran on windows/x64

MinOpts (-0.00% to +0.01%)
Collection PDIFF
libraries.pmi.windows.arm64.checked.mch +0.01%

Details here


@ryujit-bot
Copy link

Diff results for #97814

Throughput diffs

Throughput diffs for windows/arm64 ran on linux/x64

MinOpts (-0.01% to +0.00%)
Collection PDIFF
libraries.pmi.windows.arm64.checked.mch -0.01%

Details here


@ryujit-bot
Copy link

Diff results for #97814

Throughput diffs

Throughput diffs for windows/arm64 ran on windows/x64

MinOpts (-0.01% to +0.00%)
Collection PDIFF
libraries.pmi.windows.arm64.checked.mch -0.01%

Details here


@kunalspathak
Copy link
Member

All failures are known:

image

@kunalspathak kunalspathak merged commit b449152 into dotnet:main Feb 7, 2024
202 of 208 checks passed
@kunalspathak
Copy link
Member

Test results on SVE enabled machine running HardwareIntrinsics_Arm_r.sh:

14:29:58.994 Running test: JIT/HardwareIntrinsics/Arm/Sve/Sve_r/Sve_r.dll
Supported ISAs:
  AdvSimd:   True
  Aes:       True
  ArmBase:   True
  Crc32:     True
  Dp:        True
  Rdm:       True
  Sha1:      True
  Sha256:    True
  Sve:       True

Beginning scenario: RunBasicScenario_Load
Beginning scenario: RunReflectionScenario_Load
14:29:59.005 Passed test: JIT/HardwareIntrinsics/Arm/Sve/Sve_r/Sve_r.dll

It just occurred to me that this should return false on windows/arm64 SVE machine until windows support is not added.

@tannergooding
Copy link
Member

It just occurred to me that this should return false on windows/arm64 SVE machine until windows support is not added.

Shouldn't that already fall out from the VM support?

@tannergooding
Copy link
Member

@kunalspathak
Copy link
Member

We set it for Unix here: https://github.com/dotnet/runtime/blob/main/src/native/minipal/cpufeatures.c#L362-L363

and never set it for Windows, as no way to check the API exists yet: https://github.com/dotnet/runtime/blob/main/src/native/minipal/cpufeatures.c#L406-L439

perfect. Thanks for confirming.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.Intrinsics arm-sve Work related to arm64 SVE/SVE2 support community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants