-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[mono] Fix vector class retrieval and type checks for binary operand APIs #107388
[mono] Fix vector class retrieval and type checks for binary operand APIs #107388
Conversation
Tagging subscribers to this area: @fanyang-mono, @steveisok |
/azp run runtime-extra-platforms, runtime-llvm |
Azure Pipelines successfully started running 2 pipeline(s). |
I'm wondering do we have any detection of intensified methods which do not take the correct code path. How did we find this issue, perf triage? |
I'm not aware of any code analysis tool that would shows us this information. I suppose we could profile the AOT compilation to verify that we're hitting the
I found this issue while reviving the fullAOT-llvm arm64, there is one runtime test case ( |
We're supposed to have tests covering all the overloads, but some of the APIs or overloads may have been missed given the sheer scope of things, general timing of when various APIs were added, or may have only partial coverage. I believe in the case of multiply here, there is partial coverage and its possible that since Mono doesn't have a "unified" implementation and takes different paths for different types and different namespaces that may have caused it to be missed in this case. |
/azp run runtime-llvm |
Azure Pipelines successfully started running 1 pipeline(s). |
…/runtime into fix-multiply-scalar-vector
/azp run runtime-llvm |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
src/mono/mono/mini/simd-intrinsics.c
Outdated
@@ -396,11 +396,11 @@ emit_simd_ins_for_binary_op (MonoCompile *cfg, MonoClass *klass, MonoMethodSigna | |||
case SN_op_Multiply: { | |||
const char *class_name = m_class_get_name (klass); | |||
if (strcmp ("Quaternion", class_name) && strcmp ("Plane", class_name)) { | |||
if (!type_is_simd_vector (fsig->params [1])) | |||
if (!type_is_simd_vector (fsig->params [1])) // vector * scalar |
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.
nit: The checks used here and elsewhere aren't really consistent
Some places are only checking the second parameter is not SIMD, others are checking it is explicitly a primitive. Likewise other checks are validating that both parameters are vector, etc.
It would be nice to be consistent here and do the right checks in all scenarios. That is, we should have at least one vector type and for the cases that accept scalars, the scalar should be a supported primitive type (not all primitive types are supported).
The intrinsics that support scalars in either position (multiply) or only in one position (divide for the second parameter) should likely be explicitly separate as well. There's some code paths, like on L2067 below, where the handling for op1 to be scalar is also being done for MinNative, Subtract, and other intrinsics where it should never be valid.
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.
That's a good point. I refactored both emit_simd_ins_for_binary_op
and the callsite in emit_sri_vector
to check for scalars and vectors for respective APIs. I made a bit large change to the emit_simd_ins_for_binary_op
as I found it easier to orient when grouped by OP code rather than by type.
Let me know what you think about the changes.
Change the function to be split by the OP code rather than the type of the operands. Add type checks to the callsite to ensure that the operands are of the correct type.
…untime into fix-multiply-scalar-vector
/azp run runtime-llvm |
Azure Pipelines successfully started running 1 pipeline(s). |
/ba-g WASM-debugger tests are known and hard to catch with known issue. They will be migrated from runtime soon, @thaystg @ilonatommy |
…APIs (dotnet#107388) - change the function to be split by the OP code rather than the type of the operands - add type checks to the callsite to ensure that the operands are of the correct type
Bug description
On Arm64, currently
Vector[64/128].Multiply(scalar, vector)
fails because we used the first argument to retrieve the vector class which in this case fails because it's not a vector.Class was previously retrieved as a class of the first argument:
runtime/src/mono/mono/mini/simd-intrinsics.c
Line 1995 in 88f9aba
which in this case is a scalar value not a vector. This caused a crash when we verify
is_element_type_primitive
runtime/src/mono/mono/mini/simd-intrinsics.c
Lines 2068 to 2069 in 2358c59
because it expects vector type.
This issue wasn't identified before because we are missing FullAOT-arm64 runtime tests coverage, and this scenario is only tested in https:/dotnet/runtime/blob/main/src/tests/JIT/Regression/JitBlue/Runtime_93876/Runtime_93876.cs
Fix
For binary operations, if the first operand is not a vector, we extract the vector type from the second operand. We also extend the fallback condition to both operands.
Additionally, full refactor of
emit_simd_ins_for_binary_op
to split the function by the OP code rather than the type of the operands.