[mono] Fix vector class retrieval and type checks for binary operand APIs

[matouskozak]

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

MonoClass* klass = fsig->param_count > 0 ? args[0]->klass : cmethod->klass;

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

	if (!is_element_type_primitive (fsig->params [0]))
	return NULL;

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://github.com/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.

[dotnet-policy-service]

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

[matouskozak]

/azp run runtime-extra-platforms, runtime-llvm

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jkurdek]

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?

[matouskozak]

I'm wondering do we have any detection of intensified methods which do not take the correct code path.

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 simd-intrinsics.c codepaths.

How did we find this issue, perf triage?

I found this issue while reviving the fullAOT-llvm arm64, there is one runtime test case (Runtime_93876) that tests this scenario. I'm not sure why we don't test this as part of libraries tests for vectors from System.Runtime.Intrinsics, but we do test it for vectors from System.Numerics. I suppose that the codegen on CoreCLR side is similar for both Runtime.Intrinsics and Numerics vectors and it was decided that testing one is enough. Do you know @tannergooding?

[tannergooding]

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.

[matouskozak]

/azp run runtime-llvm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[matouskozak]

/azp run runtime-llvm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[matouskozak]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tannergooding]

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.

[matouskozak]

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.

[matouskozak]

/azp run runtime-llvm

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).