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

[mono] Fix Vector<T>.IsSupported intrinsic #90023

Merged
merged 7 commits into from
Aug 14, 2023

Conversation

ivanpovazan
Copy link
Member

This PR fixes a regression introduced in: #86546 where Vector<T>.IsSupported tests started failing on win-x64 platform.

The Vector<T>.IsSupported is now intrinsified properly when the type parameter T is a primitive type on all supported
platforms.


Fixes #88983

@ivanpovazan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lewing
Copy link
Member

lewing commented Aug 4, 2023

This appears to fail for MONO_TYPE_CHAR? which is causing problems in the tests, is there a reason we aren't allowing MONO_TYPE_CHAR in MONO_TYPE_IS_VECTOR_PRIMITIVE?

@vargaz
Copy link
Contributor

vargaz commented Aug 4, 2023

Vector128.IsSupported returns false for char.

@lewing
Copy link
Member

lewing commented Aug 4, 2023

Ah yeah, here is one of the wasm failures

        [wasm test-browser] fail: MONO_WASM:    at System.Text.Json.JsonReaderHelper.GetUtf8ByteCount(ReadOnlySpan`1 )
        [wasm test-browser]          at System.Text.Json.JsonDocument.Parse(ReadOnlyMemory`1 , JsonDocumentOptions )
        [wasm test-browser]          at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object , BindingFlags )
        [wasm test-browser]       Error: Specified type is not supported
        [wasm test-browser]           at sr (http://127.0.0.1:44753/_framework/dotnet.runtime.js:3:33283)
        [wasm test-browser]           at Br (http://127.0.0.1:44753/_framework/dotnet.runtime.js:3:42678)
        [wasm test-browser]           at p.javaScriptExports.call_entry_point (http://127.0.0.1:44753/_framework/dotnet.runtime.js:3:199925)
        [wasm test-browser]           at Object.Il [as runMain] (http://127.0.0.1:44753/_framework/dotnet.runtime.js:3:166662)
        [wasm test-browser]           at run (http://127.0.0.1:44753/test-main.js:390:50)
        [wasm test-browser]           at async http://127.0.0.1:44753/test-main.js:406:1

i

@lewing lewing force-pushed the fix-vector128-issupported branch from eaf88e5 to 1cc0b3e Compare August 4, 2023 21:16
@lewing
Copy link
Member

lewing commented Aug 4, 2023

I fixed the throwhelpers to share the same logic.

@matouskozak
Copy link
Member

LGTM.

I see that the runtime-extra-platforms test that was failing before windows-x64 Release AllSubsets_Mono in #88983 started failing in a different way (I already saw it fail on main before so not caused by this PR).

@ivanpovazan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@premun
Copy link
Member

premun commented Aug 9, 2023

Just an FYI, we've seen a spike of failed jobs on OSX 13 AppleTV queue originating from this PR:

WorkItems
| where QueueName == "osx.13.amd64.appletv.open" and Finished > now() - 3d and Finished < now() - 1d
| where ExitCode != 0
| join kind=inner Jobs on JobId
| summarize count() by Source

This happened about 2 days ago and was enough to trigger an alert for our infra.
I am not sure if there wasn't a unrelated infra issue but I suggest re-running this PR on an appletv queue.

@ivanpovazan
Copy link
Member Author

@SamMonoRT If I don't manage to resolve the failures introduced by this PR, we can disable the failing Vector128 tests on win-x64 to unblock clean runtime-extra-platforms pipeline for RC1, and fix the issue immediately after it.

@ivanpovazan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan
Copy link
Member Author

I have opened several tracking issues for failures on runtime-extra-platforms none of which are related to these changes.

This PR now seems to be a proper fix - originally reported regressions on win-x64 are gone.

@vargaz @lewing @SamMonoRT do you want to take another look before I merge this?

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.

LGTM!

@ivanpovazan ivanpovazan merged commit 715086f into dotnet:main Aug 14, 2023
@ivanpovazan ivanpovazan deleted the fix-vector128-issupported branch August 15, 2023 09:41
@ghost ghost locked as resolved and limited conversation to collaborators Sep 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants