[Mono] Add arm64 SIMD intrinsic for Vector64/128 Abs #65086
Conversation
mkhamoyan
requested review from
imhameed,
lambdageek,
SamMonoRT and
vargaz
as code owners
mkhamoyan
requested review from
fanyang-mono,
simonrozsival,
vargaz,
SamMonoRT,
imhameed and
lambdageek
and removed request for
vargaz,
lambdageek,
SamMonoRT and
imhameed
src/mono/mono/mini/simd-intrinsics.c
Outdated
| case SN_Abs: { | ||
| #ifdef TARGET_ARM64 | ||
| MonoType *arg_type = get_vector_t_elem_type (fsig->params [0]); | ||
| if (!MONO_TYPE_IS_INTRINSICS_VECTOR_PRIMITIVE (arg_type)) |
There was a problem hiding this comment.
I am not sure if this check is necessary. What would happen if you don't have this?
There was a problem hiding this comment.
I wanted to check if type is numeric but I see now that it doesn't check that. Do we have some function to understand if type is numeric?
There was a problem hiding this comment.
I had the impression that it is guaranteed that it should be numeric type, cause the other cases wasn't checking this.
There was a problem hiding this comment.
It validates that T is actually a supported type for Vector64/128/256 as not every primitive type is supported.
Notably, it can be relaxed from
runtime/src/mono/mono/mini/simd-intrinsics.c
Line 599 in 04bb6eb
To just:
- #define MONO_TYPE_IS_INTRINSICS_VECTOR_PRIMITIVE(t) ((MONO_TYPE_IS_VECTOR_PRIMITIVE(t)) && ((t)->type != MONO_TYPE_I) && ((t)->type != MONO_TYPE_U))
+ #define MONO_TYPE_IS_INTRINSICS_VECTOR_PRIMITIVE(t) MONO_TYPE_IS_VECTOR_PRIMITIVE(t)Where MONO_TYPE_IS_VECTOR_PRIMITIVE is:
runtime/src/mono/mono/mini/mini.h
Line 142 in 6c72db8
For reference; in .NET 6 Vector64/128/256<T> only support:
byteandsbyteshortandushortintanduintlongandulongfloatanddouble
While Vector<T> supports those plus nint and nuint.
In .NET 7, Vector64/128/256<T> also support nint and nuint bringing them inline.
We do not support bool, char, decimal, or half today (or any of the other types that may be built-in or specially recognized).
There was a problem hiding this comment.
I see, thanks for the detailed explanation.
There was a problem hiding this comment.
@tannergooding So it is not guaranteed that runtime would only be given the supported types?
There was a problem hiding this comment.
Right. A user can go declare Vector<bool> just fine, nothing prevents that. Same with Vector<Guid>, etc
Attempting to use any of the methods/members on Vector<bool> is expected to fail however. So the JIT shouldn't intrinsify them and should let the managed type checks throw the relevant exception
There was a problem hiding this comment.
Created an issue #65318 to track the cleanup work.
src/mono/mono/mini/simd-intrinsics.c
Outdated
| @@ -635,6 +636,18 @@ emit_sri_vector (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSignature *fsi | |||
| MonoTypeEnum arg0_type = fsig->param_count > 0 ? get_underlying_type (fsig->params [0]) : MONO_TYPE_VOID; | |||
|
|
|||
| switch (id) { | |||
| case SN_Abs: { | |||
| #ifdef TARGET_ARM64 | |||
| MonoType *arg_type = get_vector_t_elem_type (fsig->params [0]); | |||
src/mono/mono/mini/simd-intrinsics.c
Outdated
| MonoType *arg_type = get_vector_t_elem_type (fsig->params [0]); | ||
| if (!MONO_TYPE_IS_INTRINSICS_VECTOR_PRIMITIVE (arg_type)) | ||
| return NULL; | ||
| gboolean is_float = type_is_float (fsig->params [0]); |
There was a problem hiding this comment.
It is faster to make type comparison directly here, since you have arg0_type already. If you look at the content of type_is_float, that's exactly what it does but with more calls to get arg0_type.
| gboolean is_float = type_is_float (fsig->params [0]); | |
| gboolean is_float = arg0_type == MONO_TYPE_R4 || arg0_type == MONO_TYPE_R8; |
fanyang-mono
added
area-Codegen-LLVM-mono
area-Codegen-meta-mono
and removed
area-Codegen-JIT-mono
labels
ghost
locked as resolved and limited conversation to collaborators
This PR contributes to #64072
Add intrinsics for Vector64/128 Abs.