-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 to handle various other vector bitcast APIs #104049
Conversation
Tagging subscribers to this area: @steveisok, @lambdageek |
At a glance this all looks good. If the regression still isn't gone after this on wasm, I can help implement the new intrinsics in the jiterpreter. |
I'm not sure why The mono interp handling for |
#104028 should make it better. All ios/tvos device runs were failing before this. |
Glad to know it wasn't something on my end, hopefully this is green and can get merged as it should give us some nice improvements 🎉 |
3d8a019
to
434ff3d
Compare
@@ -283,47 +304,49 @@ emit_common_simd_operations (TransformData *td, int id, int atype, int vector_si | |||
} | |||
|
|||
static gboolean | |||
get_common_simd_info (MonoClass *vector_klass, MonoMethodSignature *csignature, MonoTypeEnum *atype, int *vector_size, int *arg_size, int *scalar_arg) | |||
get_common_simd_info (MonoClass *klass, MonoTypeEnum *atype, int *klass_size, int *arg_size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file ended up needing a bit of a refactoring as there were some assumptions in place that don't hold when supporting additional intrinsics.
In particular, there are various intrinsics where:
- one of the SIMD types may not be generic at all (
Vector2
,Vector3
,Vector4
) - multiple generic types exist (
As<TFrom, TTo>
) - the return type may not be a 128-bit vector (
AsVector2
,AsVector3
)
So, what I did here was I broke this get_common_simd_info
method into two:
get_common_simd_info
get_common_simd_scalar_arg
The former now always gets the size of the input klass and secondly determines if it is a SIMD type and what the underlying element type is if so. While the latter identifies the first non-SIMD argument, if one exists.
} | ||
|
||
static void | ||
emit_common_simd_epilogue (TransformData *td, MonoClass *vector_klass, MonoMethodSignature *csignature, int vector_size, gboolean allow_void) | ||
emit_common_simd_epilogue (TransformData *td, MonoMethodSignature *csignature) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To support methods which return a value type, but where that isn't a 128-bit vector, this explicitly gets the return type from the signature to ensure there can be no accidents
@@ -375,124 +395,246 @@ emit_sri_vector128 (TransformData *td, MonoMethod *cmethod, MonoMethodSignature | |||
gint16 simd_opcode = -1; | |||
gint16 simd_intrins = -1; | |||
|
|||
vector_klass = mono_class_from_mono_type_internal (csignature->ret); | |||
MonoTypeEnum ret_atype; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In here, we consistently query the relevant simd information of the return type and if it exists the simd information of the first parameter (this is enough to correctly handle all the cases that currently exist).
There's actually quite a bit of logic in this function that could be moved down into emit_common_simd_operations
, as APIs like AndNot
exist for Vector128<T>
and Vector<T>
, they may also exist for types like Vector4
in the future. I opted to not move that down in this PR, to try and keep the total churn under control.
But, I did add some basic validation that the encountered signatures are roughly as expected to help ensure we don't hit issues in the future as new overloads are introduced or the general SIMD support in Mono is expanded.
742333c
to
f35dd63
Compare
f35dd63
to
4cd5d90
Compare
4cd5d90
to
e4a14fb
Compare
09ee415
to
1d03cbd
Compare
1d03cbd
to
8b0b476
Compare
Similarly to #103915, I don't have the time to continue to try and debug why these changes aren't working for Mono. Anyone should feel free to pick this up if they have more context or time. -- As it stands, there appears to be some quirk where Mono is either incorrectly doing comparisons or other handling around |
This should resolve dotnet/perf-autofiling-issues#36484