[mono] RuntimeHelpers.CreateSpan<T> is now intrinsic. #81695
Conversation
jandupej
requested review from
vargaz,
lambdageek and
SamMonoRT
as code owners
|
/azp run extra-platforms |
|
No pipelines are associated with this pull request. |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Some extra-platforms failures are due to #80976 |
| // 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; |
There was a problem hiding this comment.
This looks a little brittle, it depends on OP_VMOVE never having its inst_p1 set otherwise.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/mono/mono/mini/intrinsics.c
Outdated
| int alignment = 0; | ||
| int element_size = mono_type_size (t, &alignment); | ||
|
|
||
| #if G_BYTE_ORDER == G_LITTLE_ENDIAN |
There was a problem hiding this comment.
This only needs to be called in the non-aot case.
src/mono/mono/mini/intrinsics.c
Outdated
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see there is also MONO_PATCH_INFO_RVA, maybe it would be more appropriate.
src/mono/mono/mini/intrinsics.c
Outdated
| int alignment = 0; | ||
| int element_size = mono_type_size (t, &alignment); | ||
|
|
||
| #if G_BYTE_ORDER == G_LITTLE_ENDIAN |
There was a problem hiding this comment.
| #if G_BYTE_ORDER == G_LITTLE_ENDIAN | |
| #if G_BYTE_ORDER == G_LITTLE_ENDIAN |
There was a problem hiding this comment.
I have moved the preprocessor directives.
src/mono/mono/mini/intrinsics.c
Outdated
|
|
||
| #if G_BYTE_ORDER == G_LITTLE_ENDIAN | ||
| int swizzle = 1; | ||
| #else |
src/mono/mono/mini/intrinsics.c
Outdated
| int swizzle = 1; | ||
| #else | ||
| int swizzle = element_size; | ||
| #endif |
| @@ -10820,9 +10820,12 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b | |||
| } else { | |||
| EMIT_NEW_PCONST (cfg, ins, handle); | |||
| } | |||
|
|
|||
| // 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; |
There was a problem hiding this comment.
The !field etc. checks should not be needed any more.
ghost
locked as resolved and limited conversation to collaborators
This addresses #80762.