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

Disable load/store vector APIs until Mono adds support for it #96944

Merged
merged 5 commits into from
Jan 16, 2024

Conversation

kunalspathak
Copy link
Member

Load/Store vector APIs were added in #84510 and the plan was to implement them in mono as part of #93081. However, since it is not in mono yet, when we started using it in #95513, they started throwing PlatformNotSupportedException. It was not caught in #95513 because the mono CI pipeline that test the AdvSimd is only part of llvm-aot which is run as part of runtime-extra-platforms. Given that Preview1 will be out in next couple of weeks, it will be incorrect for Mono to return AdvSimd.IsSupported == true, but throw PlatformNotSupportedException for these APIs. Working with Mono team offline, their plan is to implement them on mono side by Preview2, but until then, for Preview 1, we decided to disable them. This PR contains following changes:

Once mono implementation is complete, we should revert the changes from this PR as well as enable these tests for mono. One example is:

[ActiveIssue("https://github.com/dotnet/runtime/pull/92855#issuecomment-1746078670", TestRuntimes.Mono)]

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 Jan 13, 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

Load/Store vector APIs were added in #84510 and the plan was to implement them in mono as part of #93081. However, since it is not in mono yet, when we started using it in #95513, they started throwing PlatformNotSupportedException. It was not caught in #95513 because the mono CI pipeline that test the AdvSimd is only part of llvm-aot which is run as part of runtime-extra-platforms. Given that Preview1 will be out in next couple of weeks, it will be incorrect for Mono to return AdvSimd.IsSupported == true, but throw PlatformNotSupportedException for these APIs. Working with Mono team offline, their plan is to implement them on mono side by Preview2, but until then, for Preview 1, we decided to disable them. This PR contains following changes:

Once mono implementation is complete, we should revert the changes from this PR as well as enable these tests for mono. One example is:

[ActiveIssue("https://github.com/dotnet/runtime/pull/92855#issuecomment-1746078670", TestRuntimes.Mono)]

Author: kunalspathak
Assignees: kunalspathak
Labels:

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

Milestone: -

@kunalspathak
Copy link
Member Author

@tannergooding @fanyang-mono @SamMonoRT @lambdageek @dotnet/arm64-contrib @JulieLeeMSFT

@kunalspathak
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kunalspathak kunalspathak marked this pull request as ready for review January 15, 2024 05:52
@kunalspathak
Copy link
Member Author

@fanyang-mono - could you please double check if the failures from runtime-extra-platforms are not because of this PR and are known?

@a74nh
Copy link
Contributor

a74nh commented Jan 15, 2024

Have the automatic performance tests run since this patch went in? If not, it'd be good to wait until they are so we can see the impact before merging this.

their plan is to implement them on mono side by Preview2, but until then, for Preview 1, we decided to disable them

@SwapnilGaikwad's upcoming work on DecodeToUtf8 should be pushed out until then.

@kunalspathak
Copy link
Member Author

Have the automatic performance tests run since this patch went in? If not, it'd be good to wait until they are so we can see the impact before merging this.

It did run - dotnet/perf-autofiling-issues#27114

Copy link
Member

@fanyang-mono fanyang-mono left a comment

Choose a reason for hiding this comment

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

Thanks for making this change!

@fanyang-mono
Copy link
Member

None of the test failures on runtime-extra-platforms are related to this PR.

@kunalspathak kunalspathak merged commit 6dab58f into dotnet:main Jan 16, 2024
229 of 240 checks passed
@kunalspathak kunalspathak deleted the load-store-mono branch January 16, 2024 15:55
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
…#96944)

* Revert "[libs] Skip AdvSimdEncode on Mono (dotnet#96829)"

This reverts commit 1a76e37.

* Revert "Use multi-reg load/store for EncodeToUtf8 (dotnet#95513)"

This reverts commit fdb03ca.

* Wrap load/store vector APIs in '#if false'

* Disable load/store vector tests

* remove the trailing space
@github-actions github-actions bot locked and limited conversation to collaborators Feb 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants