[mono] RuntimeHelpers.CreateSpan<T> is now intrinsic. #81695

jandupej · 2023-02-06T13:29:19Z

This addresses #80762.

…fer way.

jandupej · 2023-02-08T16:16:37Z

/azp run extra-platforms

azure-pipelines · 2023-02-08T16:16:55Z

No pipelines are associated with this pull request.

jandupej · 2023-02-08T16:25:33Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-02-08T16:25:56Z

Azure Pipelines successfully started running 1 pipeline(s).

jandupej · 2023-02-09T14:33:46Z

Some extra-platforms failures are due to #80976

vargaz · 2023-02-09T14:50:03Z

src/mono/mono/mini/intrinsics.c

+			// The following relies on the previous instruction being an OP_MOVE. Specifically. one that
+			// is emitted from CEE_TDTOKEN as the last EMIT_NEW_TEMPLOAD, which has its inst_p1 set.
+			// Bail out and use the non-intrinsic variant if this is not satisfied.
+			MonoClassField* field = (MonoClassField*) args [0]->inst_p1;


This looks a little brittle, it depends on OP_VMOVE never having its inst_p1 set otherwise.

I guess I could also use backend; the comments there do not indicate that the field is used with OP_VMOVE. Another way would be to do what you suggested earlier - emit a new opcode that would carry it in inst_p1 and then decompose it away. I had some issues getting that approach to work, so I tried this. It seems to work, although I cannot honestly verify that its inst_p1 is always untouched. Wouldn't that also be the case for the new opcode?

Using a new opcode would mean that this code could be sure that the input comes from the CEE_LDTOKEN code. The downside is that new opcode needs to be decomposed/lowered if its not followed by CreateSpan etc.

So now a OP_LDTOKEN_FIELD is emitted, which is equivalent in every way to a OP_VMOVE, except its opcode. This should protect the MonoInst contents until it is used by CreateSpan intrinsic. The decomposition of OP_LDTOKEN_FIELD is then only rewriting the opcode to OP_VMOVE.

vargaz · 2023-02-09T14:51:37Z

src/mono/mono/mini/intrinsics.c

+			int alignment = 0;
+			int element_size = mono_type_size (t, &alignment);
+
+		#if G_BYTE_ORDER == G_LITTLE_ENDIAN


This only needs to be called in the non-aot case.

vargaz · 2023-02-09T14:52:29Z

src/mono/mono/mini/intrinsics.c

+			MonoClassField* field_ref = mono_class_get_field_from_name_full (span->klass, "_reference", NULL);
+			MonoInst* ptr_inst;
+			if (cfg->compile_aot) {
+				EMIT_NEW_SFLDACONST (cfg, ptr_inst, field);


Will this work for these kinds of fields ? At runtime, the code in mono_resolve_patch_target_ext () gets executed to retrieve the field address, i.e. the
MONO_PATCH_INFO_SFLDA case.

I see there is also MONO_PATCH_INFO_RVA, maybe it would be more appropriate.

fanyang-mono · 2023-02-09T15:25:32Z

src/mono/mono/mini/intrinsics.c

+			int alignment = 0;
+			int element_size = mono_type_size (t, &alignment);
+
+		#if G_BYTE_ORDER == G_LITTLE_ENDIAN


Suggested change

#if G_BYTE_ORDER == G_LITTLE_ENDIAN

#if G_BYTE_ORDER == G_LITTLE_ENDIAN

I have moved the preprocessor directives.

fanyang-mono · 2023-02-09T15:26:06Z

src/mono/mono/mini/intrinsics.c

+
+		#if G_BYTE_ORDER == G_LITTLE_ENDIAN
+			int swizzle = 1;
+		#else


fanyang-mono · 2023-02-09T15:26:15Z

src/mono/mono/mini/intrinsics.c

+			int swizzle = 1;
+		#else
+			int swizzle = element_size;
+		#endif


fanyang-mono · 2023-02-09T15:27:02Z

src/mono/mono/mini/method-to-ir.c

 					} else {
 						EMIT_NEW_PCONST (cfg, ins, handle);
 					}
+


Suggested change

…ateSpan<T>

…ect AOT constant.

vargaz · 2023-02-11T18:13:11Z

src/mono/mono/mini/intrinsics.c

+			// This OP_LDTOKEN_FIELD later changes into a OP_VMOVE.
+			MonoClassField* field = (MonoClassField*) args [0]->inst_p1;
+			if (args [0]->opcode != OP_LDTOKEN_FIELD || !field || !field->type)
+					return NULL;


The !field etc. checks should not be needed any more.

…y checks.

jandupej added 3 commits January 31, 2023 14:15

[mono] RuntimeHelpers.CreateSpan is now intrinsic.

789a6f7

[mono][jit] RuntimeHelpers.CreateSpan intrinsic fixes.

b76e424

[mono][jit] RuntimeHelpers.CreateSpan is intrinsic and seems to work.

5c23aa3

ghost assigned jandupej Feb 6, 2023

ghost added the area-Codegen-JIT-mono label Feb 6, 2023

jandupej added 4 commits February 6, 2023 15:28

[mono] Fixed a warning that broke build.

ff717ca

[mono] Pointer arithmetic warnings hopefully solved.

14dba0f

[mono] Fix pointer conversion warning, take 2.

9882de0

[mono] RuntimeHelpers.CreateSpan<T> is no longer intrinsic under AOT.

aad9e93

build-analysis bot mentioned this pull request Feb 7, 2023

Roslyn source generator crash on mono/linux/arm64 #81123

Closed

jandupej added 2 commits February 8, 2023 11:02

[mono] RuntimeHelpers.CreateSpan<T> now constructs the result in a sa…

1174cb5

…fer way.

[mono][aot] RuntimeHelpers.CreateSpan<T> intrinsic fixed for AOT case.

e831cdb

jandupej marked this pull request as ready for review February 8, 2023 13:34

jandupej requested review from SamMonoRT, lambdageek and vargaz as code owners February 8, 2023 13:35

jandupej marked this pull request as draft February 8, 2023 13:35

jandupej force-pushed the issue80762 branch from 585db85 to e831cdb Compare February 8, 2023 13:56

[mono] Cleaned up whitespace changes in decompose.c, YAGNI in ir-emit.h.

fe79643

This was referenced Feb 8, 2023

Tracking issue for CI build timeouts #76454

Closed

NuGet failing with Response status code does not indicate success: 503 (Service Unavailable) dotnet/arcade#11723

Open

jandupej marked this pull request as ready for review February 8, 2023 14:02

vargaz reviewed Feb 9, 2023

View reviewed changes

fanyang-mono reviewed Feb 9, 2023

View reviewed changes

jandupej added 2 commits February 10, 2023 14:32

[mono] OP_LDTOKEN_FIELD mini opcode introduced for RuntimeHelpers.Cre…

c55bbdf

…ateSpan<T>

[mono][aot] RuntimeHelpers.CreateSpan<T> intrinsic now emits the corr…

619cba6

…ect AOT constant.

vargaz reviewed Feb 11, 2023

View reviewed changes

vargaz approved these changes Feb 11, 2023

View reviewed changes

[mono] RuntimeHelpers.CreateSpan intrinsic, removed superfluous sanit…

0e03cf0

…y checks.

jandupej merged commit 7e154e1 into dotnet:main Feb 13, 2023

jandupej deleted the issue80762 branch February 13, 2023 12:37

jandupej mentioned this pull request Feb 13, 2023

Intrinsify RuntimeHelpers.CreateSpan on Mono #80762

Closed

lewing mentioned this pull request Feb 21, 2023

[Perf] Linux/x64: 233 Improvements on 2/13/2023 12:37:33 PM dotnet/perf-autofiling-issues#13304

Open

radekdoulik mentioned this pull request Feb 22, 2023

[Perf] Linux/x64: 10 Improvements on 2/13/2023 3:18:56 PM dotnet/perf-autofiling-issues#13305

Open

ghost locked as resolved and limited conversation to collaborators Mar 15, 2023

	#if G_BYTE_ORDER == G_LITTLE_ENDIAN
	#if G_BYTE_ORDER == G_LITTLE_ENDIAN

[mono] RuntimeHelpers.CreateSpan<T> is now intrinsic. #81695

[mono] RuntimeHelpers.CreateSpan<T> is now intrinsic. #81695

Uh oh!

Conversation

jandupej commented Feb 6, 2023

Uh oh!

jandupej commented Feb 8, 2023

Uh oh!

azure-pipelines bot commented Feb 8, 2023

Uh oh!

jandupej commented Feb 8, 2023

Uh oh!

azure-pipelines bot commented Feb 8, 2023

Uh oh!

jandupej commented Feb 9, 2023

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants