Ensure the other compilers consistently handle both get_IsHardwareAccelerated and get_IsSupported#128492
Ensure the other compilers consistently handle both get_IsHardwareAccelerated and get_IsSupported#128492tannergooding wants to merge 5 commits into
Conversation
…elerated and get_IsSupported
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
There was a problem hiding this comment.
Pull request overview
This PR updates multiple runtime backends (Mono mini JIT, Mono interpreter, and CoreCLR interpreter) to recognize and fold both get_IsHardwareAccelerated and get_IsSupported for vector APIs, and refactors CoreLib’s Vector*<T>.IsSupported implementations to delegate to Scalar<T>.IsSupported.
Changes:
- Mono mini JIT: return a constant
falseforVector256/Vector512.*.get_IsHardwareAcceleratedinstead of falling back to managed. - Mono interpreter + CoreCLR interpreter: add/extend intrinsic recognition to fold
get_IsSupported(and keep foldingget_IsHardwareAccelerated). - CoreLib: replace per-type
typeof(T) == ...chains inVector64/128/256/512<T>.IsSupportedwithScalar<T>.IsSupported.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mono/mono/mini/simd-intrinsics.c | Adds constant-folding for get_IsHardwareAccelerated on unsupported vector sizes (256/512). |
| src/mono/mono/mini/interp/transform.c | Extends interpreter intrinsic handling to fold get_IsSupported for relevant Vector APIs. |
| src/mono/mono/mini/interp/transform-simd.c | Expands SIMD intrinsic handling across Vector sizes and adds get_IsSupported handling. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector64_1.cs | Refactors IsSupported to use Scalar<T>.IsSupported. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector128_1.cs | Refactors IsSupported to use Scalar<T>.IsSupported. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector256_1.cs | Refactors IsSupported to use Scalar<T>.IsSupported. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Vector512_1.cs | Refactors IsSupported to use Scalar<T>.IsSupported. |
| src/coreclr/interpreter/intrinsics.cpp | Adds recognition for get_IsSupported on System.Runtime.Intrinsics.Vector* types. |
| src/coreclr/interpreter/compiler.cpp | Implements NI_IsSupported_Type expansion to a constant based on the generic type argument. |
Comments suppressed due to low confidence (1)
src/mono/mono/mini/interp/transform-simd.c:552
emit_sri_vectoronly identifies the vector type from a generic-inst return or first parameter. For explicitISimdVector<...,T>.get_IsHardwareAccelerated(and other vector-less members), the signature has noMONO_TYPE_GENERICINST, so this returnsFALSEand the interpreter won't constant-fold the check. If the intent is to fold these calls, consider also usingcmethod->klasswhen it's a SIMD generic instance (or special-casingSN_get_IsHardwareAccelerated/SN_get_IsSupportedbefore the vector_klass inference).
gint16 simd_opcode = -1;
gint16 simd_intrins = -1;
if (csignature->ret->type == MONO_TYPE_GENERICINST) {
vector_klass = mono_class_from_mono_type_internal (csignature->ret);
} else if (csignature->param_count && csignature->params [0]->type == MONO_TYPE_GENERICINST) {
vector_klass = mono_class_from_mono_type_internal (csignature->params [0]);
} else {
return FALSE;
}
fbd0a00 to
dcbb3eb
Compare
dcbb3eb to
1067f70
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/mono/mono/mini/interp/transform-simd.c:552
- emit_sri_vector() no longer successfully handles Vector64/Vector128/Vector256/Vector512.get_IsHardwareAccelerated: those APIs return bool and have no genericinst return/arg, so the current vector_klass discovery falls into the final
return FALSE;(lines 546-552). As a result, the SIMD path won’t fold IsHardwareAccelerated totruewhen SIMD intrinsics are enabled (it will fall back to interp_handle_intrinsics, which folds it to false), and the laterif (id == SN_get_IsHardwareAccelerated) { ... MINT_LDC_I4_1 ... }becomes effectively unreachable. Consider restoring an early special-case for SN_get_IsHardwareAccelerated (before requiring a genericinst) that bases the result on the declaring Vector* type/size (e.g., true for supported sizes, false for unsupported ones).
const char *cmethod_name = strip_explicit_isimd_prefix (cmethod->name);
int id = lookup_intrins (sri_vector_methods, sizeof (sri_vector_methods), cmethod_name);
if (id == -1)
return FALSE;
MonoClass *vector_klass = NULL;
int vector_size = 0;
gint16 simd_opcode = -1;
gint16 simd_intrins = -1;
if (csignature->ret->type == MONO_TYPE_GENERICINST) {
vector_klass = mono_class_from_mono_type_internal (csignature->ret);
} else if (csignature->param_count && csignature->params [0]->type == MONO_TYPE_GENERICINST) {
vector_klass = mono_class_from_mono_type_internal (csignature->params [0]);
} else {
return FALSE;
}
74d2319 to
5c552c0
Compare
42dce9d to
a59cf9d
Compare
| g_assert (mono_class_is_ginst (target_method->klass)); | ||
|
|
||
| // Apart from filtering out non-primitive types this also filters out shared generic instance types like: T_BYTE which cannot be intrinsified | ||
| MonoType *etype = mono_class_get_context (target_method->klass)->class_inst->type_argv [0]; | ||
|
|
||
| if (MONO_TYPE_IS_VECTOR_PRIMITIVE (etype)) { | ||
| *op = MINT_LDC_I4_1; | ||
| } else if (mini_type_get_underlying_type (etype)->type == MONO_TYPE_OBJECT) { | ||
| // Happens often in gshared code |
| g_assert (mono_class_is_ginst (target_method->klass)); | ||
|
|
||
| // Apart from filtering out non-primitive types this also filters out shared generic instance types like: T_BYTE which cannot be intrinsified | ||
| MonoType *etype = mono_class_get_context (target_method->klass)->class_inst->type_argv [0]; | ||
|
|
||
| if (MONO_TYPE_IS_VECTOR_PRIMITIVE (etype)) { | ||
| *op = MINT_LDC_I4_1; | ||
| } else if (mini_type_get_underlying_type (etype)->type == MONO_TYPE_OBJECT) { | ||
| // Happens often in gshared code | ||
| *op = MINT_LDC_I4_0; |
|
There's something about mono interpreter and its handling that I don't understand here. The expansion should all be in place and it should ensure that we can do the relevant DCE, but it is still failing and causing some tests to hit a stack overflow. |
These checks frequently appear in generic and other code allowing early dead code elimination. They should be consistently handled to avoid pessimizing things.