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

[libs][iOS] AdvSimdEncode hits PlatformNotSupportedException #96828

Closed
mdh1418 opened this issue Jan 11, 2024 · 11 comments
Closed

[libs][iOS] AdvSimdEncode hits PlatformNotSupportedException #96828

mdh1418 opened this issue Jan 11, 2024 · 11 comments

Comments

@mdh1418
Copy link
Member

mdh1418 commented Jan 11, 2024

Description

After #95513 was merged, some tests that eventually invoke EncodeToUtf8 such as

public static void HeapSort_Heapify_String(string[] elements)
began failing with PlatformNotSupportedException on iOS/tvOS arm64 lanes.

Test collection for System.Collections.Tests.PriorityQueue_PropertyTests
Test name: System.Collections.Tests.PriorityQueue_PropertyTests.HeapSort_Heapify_String
Exception messages: System.PlatformNotSupportedException : 
Exception stack traces:    at System.Runtime.Intrinsics.Arm.AdvSimd.Arm64.LoadVector128x3AndUnzip(Byte* address)
at System.Buffers.Text.Base64.EncodeToUtf8(ReadOnlySpan`1 bytes, Span`1 utf8, Int32& bytesConsumed, Int32& bytesWritten, Boolean isFinalBlock)
at System.Convert.ToBase64CharsLargeNoLineBreaks(ReadOnlySpan`1 bytes, Span`1 chars, Int32 charLengthRequired)
at System.Convert.ToBase64String(ReadOnlySpan`1 bytes, Base64FormattingOptions options)
at System.Convert.ToBase64String(Byte[] inArray)
at System.Collections.Tests.PriorityQueue_PropertyTests.GenString(Random random)
at System.Collections.Tests.PriorityQueue_PropertyTests.GenArray[String](Func`2 genElement, Random random)
at System.Collections.Tests.PriorityQueue_PropertyTests.<>c.<GetRandomStringArrays>b__16_0(Random random)
at System.Collections.Tests.PriorityQueue_PropertyTests.<GenerateMemberData>d__18`1[[System.String[], System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()
at System.Linq.Enumerable.SelectEnumerableIterator`2[[System.Object, System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Object[], System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()

It looks like LoadVector128x3AndUnzip is possibly the culprit, as #84510 doesn't seem to account for mono.

Reproduction Steps

On OSX with an iOS device

./build.sh -s mono+libs -os ios -arch arm64 -c debug
./dotnet.sh build src/libraries/System.Collections/tests/System.Collections.Tests.csproj /t:Test /p:TargetOS=ios /p:TargetArchitecture=arm64 /p:Configuration=Debug /p:DevTeamProvisioning=<dev team provisioning ID> /p:XunitMethodName=System.Collections.Tests.PriorityQueue_PropertyTests.HeapSort_Heapify_String /p:MonoEnableLLVM=true /p:RunAOTCompilation=true

Expected behavior

Test doesn't fail with PlatformNotSupportedException

Actual behavior

Various tests eventually that end up calling AdvSimd.Arm64.LoadVector128x3AndUnzip on iOS/tvOS fail with PlatformNotSupportedException

Regression?

Yes

Known Workarounds

No-op AdvSimdEncode on mono

Configuration

No response

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 11, 2024
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 11, 2024
@mdh1418
Copy link
Member Author

mdh1418 commented Jan 11, 2024

@kunalspathak
Copy link
Member

kunalspathak commented Jan 11, 2024

@vargaz, @fanyang-mono - do you know what is needed on mono side? We did that previously for Store APIs in https://github.com/dotnet/runtime/pull/93223/files#diff-012b47a27abc5dbc9d2a0b6fdef4adec27fa207a7d2b53823a8bc80f6539f5af. Probably need similar changes for Load APIs?

@vargaz
Copy link
Contributor

vargaz commented Jan 11, 2024

Would suggest adding an ifdef MONO around that code until this is fixed.

@fanyang-mono
Copy link
Member

There is not software fallback codepath for AdvSimd.Arm64.LoadVector128x3AndUnzip. When it is not intrinsified by the runtime, it will throw System.PlatformNotSupportedException. Current solution would be avoid using AdvSimd.Arm64.LoadVector128x3AndUnzip and other new API's in #84510 for BCL methods when using Mono runtime, until they are implemented by Mono.

@am11 am11 added arch-arm64 area-System.Runtime.Intrinsics and removed untriaged New issue has not been triaged by the area owner needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jan 11, 2024
@ghost
Copy link

ghost commented Jan 11, 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

Description

After #95513 was merged, some tests that eventually invoke EncodeToUtf8 such as

public static void HeapSort_Heapify_String(string[] elements)
began failing with PlatformNotSupportedException on iOS/tvOS arm64 lanes.

Test collection for System.Collections.Tests.PriorityQueue_PropertyTests
Test name: System.Collections.Tests.PriorityQueue_PropertyTests.HeapSort_Heapify_String
Exception messages: System.PlatformNotSupportedException : 
Exception stack traces:    at System.Runtime.Intrinsics.Arm.AdvSimd.Arm64.LoadVector128x3AndUnzip(Byte* address)
at System.Buffers.Text.Base64.EncodeToUtf8(ReadOnlySpan`1 bytes, Span`1 utf8, Int32& bytesConsumed, Int32& bytesWritten, Boolean isFinalBlock)
at System.Convert.ToBase64CharsLargeNoLineBreaks(ReadOnlySpan`1 bytes, Span`1 chars, Int32 charLengthRequired)
at System.Convert.ToBase64String(ReadOnlySpan`1 bytes, Base64FormattingOptions options)
at System.Convert.ToBase64String(Byte[] inArray)
at System.Collections.Tests.PriorityQueue_PropertyTests.GenString(Random random)
at System.Collections.Tests.PriorityQueue_PropertyTests.GenArray[String](Func`2 genElement, Random random)
at System.Collections.Tests.PriorityQueue_PropertyTests.<>c.<GetRandomStringArrays>b__16_0(Random random)
at System.Collections.Tests.PriorityQueue_PropertyTests.<GenerateMemberData>d__18`1[[System.String[], System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()
at System.Linq.Enumerable.SelectEnumerableIterator`2[[System.Object, System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.Object[], System.Private.CoreLib, Version=9.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()

It looks like LoadVector128x3AndUnzip is possibly the culprit, as #84510 doesn't seem to account for mono.

Reproduction Steps

On OSX with an iOS device

./build.sh -s mono+libs -os ios -arch arm64 -c debug
./dotnet.sh build src/libraries/System.Collections/tests/System.Collections.Tests.csproj /t:Test /p:TargetOS=ios /p:TargetArchitecture=arm64 /p:Configuration=Debug /p:DevTeamProvisioning=<dev team provisioning ID> /p:XunitMethodName=System.Collections.Tests.PriorityQueue_PropertyTests.HeapSort_Heapify_String /p:MonoEnableLLVM=true /p:RunAOTCompilation=true

Expected behavior

Test doesn't fail with PlatformNotSupportedException

Actual behavior

Various tests eventually that end up calling AdvSimd.Arm64.LoadVector128x3AndUnzip on iOS/tvOS fail with PlatformNotSupportedException

Regression?

Yes

Known Workarounds

No-op AdvSimdEncode on mono

Configuration

No response

Other information

No response

Author: mdh1418
Assignees: -
Labels:

arch-arm64, area-System.Runtime.Intrinsics

Milestone: -

@kunalspathak
Copy link
Member

Current solution would be avoid using AdvSimd.Arm64.LoadVector128x3AndUnzip and other new API's in #84510 for BCL methods when using Mono runtime, until they are implemented by Mono

Should we create an issue to implement these or is it already captured somewhere?

@fanyang-mono
Copy link
Member

@kunalspathak I've created one to track that work. #93081

@jeffhandley
Copy link
Member

With #93081 open, is there anything more to do on this issue or can it be closed?

@jeffhandley jeffhandley added this to the 9.0.0 milestone Jul 27, 2024
@fanyang-mono
Copy link
Member

fanyang-mono commented Jul 29, 2024

@lambdageek will run this test locally to see if it is passing now. If so, we could close this issue. Since #93081 has been completed.

Thanks, @lambdageek

@lambdageek lambdageek self-assigned this Jul 29, 2024
@tannergooding
Copy link
Member

@lambdageek, were you able to complete the testing?

I did some basic testing locally and it looked to be working as expected

@tannergooding
Copy link
Member

Going to close this as the local validation shows its no longer an issue and I don't see failures in recent CI runs.

Please reopen if the issue repros anywhere else.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants