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

[Mono] [Arm64] Add SIMD support for vector ctors #98037

Merged

Conversation

jkurdek
Copy link
Member

@jkurdek jkurdek commented Feb 6, 2024

Should fix #97815

@ghost
Copy link

ghost commented Feb 6, 2024

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

Issue Details

Should fix #97815

Author: jkurdek
Assignees: -
Labels:

area-Codegen-Intrinsics-mono

Milestone: -

@jkurdek jkurdek marked this pull request as ready for review February 6, 2024 18:46
@jkurdek
Copy link
Member Author

jkurdek commented Feb 6, 2024

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

ins->inst_c0 = 2;
MonoInst* vec_ins = args [1];
if (COMPILE_LLVM (cfg)) {
vec_ins = emit_simd_ins (cfg, klass, OP_XWIDEN, args [1]->dreg, -1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good job!
I noticed that there are System.Numerics.Vectors red on linux bionic arm64 but it looks infrastructure related. Possibly re-run of CI could make them green.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked some jobs before and after changes to vector3. System.Numerics.Vectors seems to be red on all of them, along with bunch of different tests on linux bionic arm64, I think it is not related.

@jkurdek jkurdek merged commit 9dd8f52 into dotnet:main Feb 7, 2024
153 of 164 checks passed
@jkurdek jkurdek deleted the arm64-vector-composite-constructors-simd branch February 7, 2024 11:44
@vargaz
Copy link
Contributor

vargaz commented Feb 7, 2024

Testcase:

using System;
using System.Threading.Tasks;
using System.Numerics;
using System.Runtime.Intrinsics;

public class Test
{
    public static void foo (Vector3 v) {
        var v2 = new Vector4 (v, 1.0f);
    }

    public static void Main () {
        foo (Vector3.Zero);
    }
}

The problems seems to be that the code emits a OP_INSERT_R4, but that expects sreg1 to be a simd vreg, but Vector3 right now is only marked as simd on arm64, not wasm, i.e. see init_class() in mini-runtime.c.

@jkurdek
Copy link
Member Author

jkurdek commented Feb 7, 2024

Yes, we do not have simd intrinsics for Vector2 and Vector3 on WASM. I'll create a PR disabling ctors simd on WASM.

@vargaz
Copy link
Contributor

vargaz commented Feb 7, 2024

Adding a:

if (!mini_class_is_simd (cfg, mono_class_from_mono_type_internal (fsig->params [0])))
				// FIXME:
				return NULL;

to the Vector2/Vector3 cases should work around the problem.

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.

[Mono] [Arm64] Add SIMD support for vector constructors using other vectors
3 participants