[Mono] [Arm64] Add basic Vector3 SIMD intrinsics by jkurdek · Pull Request #97416 · dotnet/runtime

jkurdek · 2024-01-23T20:27:23Z

Basic SIMD support for System.Numerics.Vector3. Should fix #91456

ghost · 2024-01-23T20:27:59Z

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

Issue Details

Basic SIMD support for System.Numerics.Vector3. Should fix #91456

Author:	jkurdek
Assignees:	jkurdek
Labels:	`area-Codegen-Intrinsics-mono`
Milestone:	-

jkurdek · 2024-01-23T20:53:04Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-01-23T20:53:17Z

Azure Pipelines successfully started running 1 pipeline(s).

vargaz · 2024-01-23T21:39:12Z

Probably needs llvm support as well.

vargaz · 2024-01-29T20:40:55Z

The llvm changes look ok as well, just the formatting needs to be cleaned up to conform to the mono coding convs.

jkurdek · 2024-01-29T22:04:35Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-01-29T22:04:58Z

Azure Pipelines successfully started running 1 pipeline(s).

src/mono/mono/mini/mini.c

ivanpovazan · 2024-01-30T13:59:47Z

src/mono/mono/mini/mini-llvm.c

+
+						if (mono_class_value_size (klass, NULL) == 12) 
+							len--;


Why are we decrementing len here?

I encountered case that lhs would be V3 represented as V4(three elements + zero). And the retval would be V3.

The code tried to extract the fourth value of lhs and insert it into retval which caused an error, as retval had only three elements.

The change made it only extract and insert the first three elms if V3 was encoutered

I would add comment explaining this special case for easier comprehension.

@jkurdek thank you for the explanation.

Just for my understanding, so it basically means that in such cases LLVMGetReturnType (ctx->lmethod_type) and LLVMTypeOf (lhs) are returning a different type (as we end up having different vector sizes) ?

If we have these kind of workarounds across the mini-llvm code, I think it is ok to manually adjust the len however to be less cryptic, it might be better to have a comment explaining the special-casing (as @matouskozak suggested) and something like this instead:

// TODO: Here add explanation why handling Vector3 requires special treatment int len = mono_class_value_size (klass, NULL) == 12 ? 3 : LLVMGetVectorSize (LLVMTypeOf (lhs));

Yup, LLVMGetReturnType (ctx->lmethod_type) is a vector of three elements, whereas LLVMTypeOf (lhs) is a vector of four elements (three + zero)

Since 3 element vectors don't exist in the hardware, it might be easier to use 4 elements ones in return types etc. and only handle 3 element ones in loads/stores etc.

src/mono/mono/mini/mini.h

jkurdek · 2024-01-30T15:22:26Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-01-30T15:22:40Z

Azure Pipelines successfully started running 1 pipeline(s).

jkurdek · 2024-01-30T16:24:34Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-01-30T16:24:49Z

Azure Pipelines successfully started running 1 pipeline(s).

jkurdek · 2024-01-31T16:53:05Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-01-31T16:53:21Z

Azure Pipelines successfully started running 1 pipeline(s).

jkurdek · 2024-01-31T19:08:36Z

/azp run runtime-extra-platforms

azure-pipelines · 2024-01-31T19:08:51Z

Azure Pipelines successfully started running 1 pipeline(s).

jkurdek · 2024-02-01T09:34:06Z

Test failures seem unrelated

matouskozak

The changes look good to me, as a follow up we could intrisify more Vector3/4 constructors.

matouskozak · 2024-02-01T11:45:33Z

src/mono/mono/mini/simd-intrinsics.c

 			for (int i = 1; i < fsig->param_count; ++i)
 				ins = emit_vector_insert_element (cfg, klass, ins, MONO_TYPE_R4, args [i + 1], i, FALSE);

+			if (len == 3) {


Does this also handle the Vector3 constructor: Vector3 (Vector2, float)? As a follow up we could enable this block

runtime/src/mono/mono/mini/simd-intrinsics.c

Lines 2734 to 2762 in 93381bf

#if 0

} else if (len == 3 && fsig->param_count == 2 && fsig->params [0]->type == MONO_TYPE_VALUETYPE && fsig->params [1]->type == etype->type) {

/* Vector3 (Vector2, float) */

int dreg = load_simd_vreg (cfg, cmethod, args [0], NULL);

ins = emit_simd_ins (cfg, klass, OP_INSERT_R4, args [1]->dreg, args [2]->dreg);

ins->inst_c0 = 2;

ins->dreg = dreg;

return ins;

} else if (len == 4 && fsig->param_count == 2 && fsig->params [0]->type == MONO_TYPE_VALUETYPE && fsig->params [1]->type == etype->type) {

/* Vector4 (Vector3, float) */

int dreg = load_simd_vreg (cfg, cmethod, args [0], NULL);

ins = emit_simd_ins (cfg, klass, OP_INSERT_R4, args [1]->dreg, args [2]->dreg);

ins->inst_c0 = 3;

ins->dreg = dreg;

return ins;

} else if (len == 4 && fsig->param_count == 3 && fsig->params [0]->type == MONO_TYPE_VALUETYPE && fsig->params [1]->type == etype->type && fsig->params [2]->type == etype->type) {

/* Vector4 (Vector2, float, float) */

int dreg = load_simd_vreg (cfg, cmethod, args [0], NULL);

int sreg = args [1]->dreg;

ins = emit_simd_ins (cfg, klass, OP_INSERT_R4, sreg, args [2]->dreg);

ins->inst_c0 = 2;

ins = emit_simd_ins (cfg, klass, OP_INSERT_R4, ins->dreg, args [3]->dreg);

ins->inst_c0 = 3;

ins->dreg = dreg;

return ins;

} else {

g_assert_not_reached ();

}

#endif

to intrinsify constructors that take another vector as a parameter.

It is not supported yet, I created a new issue for the rest of vector constructors as it seems they don't work out of the box.

#97815

matouskozak · 2024-02-01T11:46:44Z

src/mono/mono/mini/mini-llvm.c

+
+						if (mono_class_value_size (klass, NULL) == 12) 
+							len--;


I would add comment explaining this special case for easier comprehension.

src/mono/mono/mini/mini.h

src/mono/mono/mini/simd-intrinsics.c

Co-authored-by: Ivan Povazan <[email protected]>

ivanpovazan

LGTM, Thank you!

[Mono] [Arm64] Add basic Vector3 SIMD intrinsics

a9918e5

jkurdek requested review from matouskozak, tannergooding and vitek-karas January 23, 2024 20:27

jkurdek self-assigned this Jan 23, 2024

jkurdek requested review from SamMonoRT, fanyang-mono, lambdageek and vargaz as code owners January 23, 2024 20:27

ghost added the area-Codegen-Intrinsics-mono label Jan 23, 2024

Merge branch 'main' into vector3-arm64-intrinsics

d384ae7

vargaz approved these changes Jan 23, 2024

View reviewed changes

Add support for LLVM

1e30142

jkurdek force-pushed the vector3-arm64-intrinsics branch from a0aa8fe to 1e30142 Compare January 29, 2024 20:20

ivanpovazan reviewed Jan 30, 2024

View reviewed changes

src/mono/mono/mini/mini.c Outdated Show resolved Hide resolved

ivanpovazan reviewed Jan 30, 2024

View reviewed changes

src/mono/mono/mini/mini.c Show resolved Hide resolved

ivanpovazan reviewed Jan 30, 2024

View reviewed changes

src/mono/mono/mini/mini.h Outdated Show resolved Hide resolved

runfoapp bot mentioned this pull request Jan 30, 2024

system.text.regularexpressions.tests work item #72834

Open

Add insert zero as last element to vector3 as vector4 ctor

dfcbb55

Fix dreg redeclaration

552c019

build-analysis bot mentioned this pull request Jan 30, 2024

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Closed

fixed const value insert

472cabd

llvm loadx zero padding + formatting + review suggestions

74a2a29

This was referenced Feb 1, 2024

[6.0] Android test failure: Failed to install mobile app #83422

Open

System.Net.Security.Tests.SslStreamCertificateContextOcspLinuxTests.RefreshOcspResponse_BeforeExpiration test failure #97779

Closed

jkurdek requested a review from ivanpovazan February 1, 2024 09:34

matouskozak approved these changes Feb 1, 2024

View reviewed changes

ivanpovazan reviewed Feb 1, 2024

View reviewed changes

src/mono/mono/mini/mini.h Outdated Show resolved Hide resolved

ivanpovazan reviewed Feb 1, 2024

View reviewed changes

src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved

ivanpovazan reviewed Feb 1, 2024

View reviewed changes

src/mono/mono/mini/simd-intrinsics.c Outdated Show resolved Hide resolved

jkurdek and others added 2 commits February 1, 2024 15:05

Apply suggestions from code review

b47cdf9

Co-authored-by: Ivan Povazan <[email protected]>

Add review suggestions

e672c53

ivanpovazan approved these changes Feb 1, 2024

View reviewed changes

build-analysis bot mentioned this pull request Feb 1, 2024

System.Net.Security.Tests.SslStreamCertificateContextOcspLinuxTests.FetchOcspResponse_FirstInvalidThenValid test failure #97836

Closed

jkurdek merged commit bdc031f into dotnet:main Feb 5, 2024

jkurdek deleted the vector3-arm64-intrinsics branch February 5, 2024 10:00

github-actions bot locked and limited conversation to collaborators Mar 7, 2024

	#if 0
	} else if (len == 3 && fsig->param_count == 2 && fsig->params [0]->type == MONO_TYPE_VALUETYPE && fsig->params [1]->type == etype->type) {
	/* Vector3 (Vector2, float) */
	int dreg = load_simd_vreg (cfg, cmethod, args [0], NULL);
	ins = emit_simd_ins (cfg, klass, OP_INSERT_R4, args [1]->dreg, args [2]->dreg);
	ins->inst_c0 = 2;
	ins->dreg = dreg;
	return ins;
	} else if (len == 4 && fsig->param_count == 2 && fsig->params [0]->type == MONO_TYPE_VALUETYPE && fsig->params [1]->type == etype->type) {
	/* Vector4 (Vector3, float) */
	int dreg = load_simd_vreg (cfg, cmethod, args [0], NULL);
	ins = emit_simd_ins (cfg, klass, OP_INSERT_R4, args [1]->dreg, args [2]->dreg);
	ins->inst_c0 = 3;
	ins->dreg = dreg;
	return ins;
	} else if (len == 4 && fsig->param_count == 3 && fsig->params [0]->type == MONO_TYPE_VALUETYPE && fsig->params [1]->type == etype->type && fsig->params [2]->type == etype->type) {
	/* Vector4 (Vector2, float, float) */
	int dreg = load_simd_vreg (cfg, cmethod, args [0], NULL);
	int sreg = args [1]->dreg;
	ins = emit_simd_ins (cfg, klass, OP_INSERT_R4, sreg, args [2]->dreg);
	ins->inst_c0 = 2;
	ins = emit_simd_ins (cfg, klass, OP_INSERT_R4, ins->dreg, args [3]->dreg);
	ins->inst_c0 = 3;
	ins->dreg = dreg;
	return ins;
	} else {
	g_assert_not_reached ();
	}
	#endif

Conversation

jkurdek commented Jan 23, 2024

Uh oh!

ghost commented Jan 23, 2024

Uh oh!

jkurdek commented Jan 23, 2024

Uh oh!

azure-pipelines bot commented Jan 23, 2024

Uh oh!

vargaz commented Jan 23, 2024

Uh oh!

vargaz commented Jan 29, 2024

Uh oh!

jkurdek commented Jan 29, 2024

Uh oh!

azure-pipelines bot commented Jan 29, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkurdek Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkurdek commented Jan 30, 2024

Uh oh!

azure-pipelines bot commented Jan 30, 2024

Uh oh!

jkurdek commented Jan 30, 2024

Uh oh!

azure-pipelines bot commented Jan 30, 2024

Uh oh!

jkurdek commented Jan 31, 2024

Uh oh!

azure-pipelines bot commented Jan 31, 2024

Uh oh!

jkurdek commented Jan 31, 2024

Uh oh!

azure-pipelines bot commented Jan 31, 2024

Uh oh!

jkurdek commented Feb 1, 2024

Uh oh!

matouskozak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivanpovazan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jkurdek Jan 30, 2024 •

edited

Loading