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

Update mono-interp to handle the same Vector.As APIs as mono-jit #104217

Merged
merged 1 commit into from
Jul 2, 2024

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.

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
Contributor

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
Contributor

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
77 of 79 checks passed
@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