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 to handle various other vector bitcast APIs #104049

Closed
wants to merge 1 commit into from

Conversation

tannergooding
Copy link
Member

Copy link
Contributor

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

@kg
Copy link
Contributor

kg commented Jun 26, 2024

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.

@tannergooding
Copy link
Member Author

tannergooding commented Jun 26, 2024

I'm not sure why ios-arm64 and tvos-arm64 are failing, I've seen this on a couple Mono PRs that are touching fairly different areas so far

The mono interp handling for AsVector2Single seems to not be quite right though, so I'll see if I can fix that first. Edit: The interp failure was a simple typo

@steveisok
Copy link
Member

I'm not sure why ios-arm64 and tvos-arm64 are failing, I've seen this on a couple Mono PRs that are touching fairly different areas so far

#104028 should make it better. All ios/tvos device runs were failing before this.

@tannergooding
Copy link
Member Author

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 🎉

@@ -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)
Copy link
Member Author

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)
Copy link
Member Author

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;
Copy link
Member Author

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.

@tannergooding tannergooding force-pushed the mono-vectoras branch 3 times, most recently from 09ee415 to 1d03cbd Compare June 28, 2024 16:04
@tannergooding
Copy link
Member Author

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 Vector3 and it's causing most of the downstream failures.

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.

[Perf] Linux/x64: 87 Regressions on 6/15/2024 2:08:36 PM
3 participants