Skip to content

Conversation

@tannergooding
Copy link
Member

@tannergooding tannergooding commented Jul 1, 2024

This is a simplified version of #104049, trying to handle the smaller set of existing scenarios to start.

@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @BrzVlad, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

@tannergooding tannergooding marked this pull request as ready for review July 1, 2024 03:43
@tannergooding
Copy link
Member Author

CC. @kg this should be ready for review

@kg
Copy link
Member

kg commented Jul 1, 2024

lgtm, but i'd feel better if vlad or milos also looked at it. if the regressions this is fixing don't go away, i can update the jiterpreter.

@tannergooding
Copy link
Member Author

if the regressions this is fixing don't go away, i can update the jiterpreter.

Sounds good! This should help some, but there's some others that still need to be handled as well. I was having troubles handling them all at once in #104049, so I decided to break it apart into smaller chunks instead starting with this one.

I'm going to get AsVector, AsQuaternion, AsPlane, AsVector4, and AsVector128(Vector4) next since those are also same size bitcasts and should be straightforward. Then that will leave AsVector128(Vector2), AsVector128(Vector3), and AsVector128Unsafe after that as the ones that change sizes and I believe were the ones causing the most issues in my previous attempt.

-- Could you give a brief explanation on the difference between the handling here and that in the jiterpreter? Does the logic not get shared between them for the SIMD acceleration?

@kg
Copy link
Member

kg commented Jul 1, 2024

if the regressions this is fixing don't go away, i can update the jiterpreter.

Sounds good! This should help some, but there's some others that still need to be handled as well. I was having troubles handling them all at once in #104049, so I decided to break it apart into smaller chunks instead starting with this one.

I'm going to get AsVector, AsQuaternion, AsPlane, AsVector4, and AsVector128(Vector4) next since those are also same size bitcasts and should be straightforward. Then that will leave AsVector128(Vector2), AsVector128(Vector3), and AsVector128Unsafe after that as the ones that change sizes and I believe were the ones causing the most issues in my previous attempt.

-- Could you give a brief explanation on the difference between the handling here and that in the jiterpreter? Does the logic not get shared between them for the SIMD acceleration?

The jiterpreter has a generic path for interpreter SIMD opcodes that dispatches directly to the C helper, but with a wasm opcode mapping or dedicated implementation (i think this would need the latter), it can generate native jitcode directly. For this bitcast I think we'd just emit a single vector memory move.

@tannergooding
Copy link
Member Author

@BrzVlad, @kotlarmilos, this should be ready for review

Copy link
Member

@kotlarmilos kotlarmilos left a comment

Choose a reason for hiding this comment

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

LGTM! @BrzVlad Please sign-off before merge.


g_assert (vector_type->type == MONO_TYPE_GENERICINST);
klass = mono_class_from_mono_type_internal (vector_type);
g_assert (
Copy link
Member

Choose a reason for hiding this comment

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

Should we also check the namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

We aren't doing so in simd-intrinsics.c, notably, but we could if desired.

I believe the expectation is that we're already only hitting this path for System.Runtime.Intrinsics or System.Numerics due to the higher level functions that can dispatch to these code paths.

@tannergooding
Copy link
Member Author

Failure is the general timeout for this leg that is already tracked.

@tannergooding tannergooding merged commit c65d921 into dotnet:main Jul 2, 2024
@tannergooding tannergooding deleted the mono-vectoras-interp branch July 2, 2024 16:59
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 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.

5 participants