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.

Sign up for GitHub

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] RuntimeHelpers.CreateSpan<T> is now intrinsic. #81695

Merged
merged 13 commits into from
Feb 13, 2023
Merged

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

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
789a6f7
[mono] RuntimeHelpers.CreateSpan is now intrinsic.
jandupej Jan 31, 2023
b76e424
[mono][jit] RuntimeHelpers.CreateSpan intrinsic fixes.
jandupej Feb 1, 2023
5c23aa3
[mono][jit] RuntimeHelpers.CreateSpan is intrinsic and seems to work.
jandupej Feb 6, 2023
ff717ca
[mono] Fixed a warning that broke build.
jandupej Feb 6, 2023
14dba0f
[mono] Pointer arithmetic warnings hopefully solved.
jandupej Feb 7, 2023
9882de0
[mono] Fix pointer conversion warning, take 2.
jandupej Feb 7, 2023
aad9e93
[mono] RuntimeHelpers.CreateSpan<T> is no longer intrinsic under AOT.
jandupej Feb 7, 2023
1174cb5
[mono] RuntimeHelpers.CreateSpan<T> now constructs the result in a sa…
jandupej Feb 8, 2023
e831cdb
[mono][aot] RuntimeHelpers.CreateSpan<T> intrinsic fixed for AOT case.
jandupej Feb 8, 2023
fe79643
[mono] Cleaned up whitespace changes in decompose.c, YAGNI in ir-emit.h.
jandupej Feb 8, 2023
c55bbdf
[mono] OP_LDTOKEN_FIELD mini opcode introduced for RuntimeHelpers.Cre…
jandupej Feb 10, 2023
619cba6
[mono][aot] RuntimeHelpers.CreateSpan<T> intrinsic now emits the corr…
jandupej Feb 10, 2023
0e03cf0
[mono] RuntimeHelpers.CreateSpan intrinsic, removed superfluous sanit…
jandupej Feb 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions src/mono/mono/mini/intrinsics.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,6 +1007,54 @@ mini_emit_inst_for_method (MonoCompile *cfg, MonoMethod *cmethod, MonoMethodSign
EMIT_NEW_BIALU_IMM (cfg, ins, OP_COMPARE_IMM, -1, dreg, 0);
EMIT_NEW_UNALU (cfg, ins, OP_ICGT, dreg, -1);
ins->type = STACK_I4;
return ins;
} else if (!strcmp (cmethod->name, "CreateSpan") && fsig->param_count == 1) {
MonoGenericContext* ctx = mono_method_get_context (cmethod);
g_assert (ctx);
g_assert (ctx->method_inst);
g_assert (ctx->method_inst->type_argc == 1);
MonoType* arg_type = ctx->method_inst->type_argv [0];
MonoType* t = mini_get_underlying_type (arg_type);
g_assert (!MONO_TYPE_IS_REFERENCE (t) && t->type != MONO_TYPE_VALUETYPE);

// 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if (args [0]->opcode != OP_VMOVE || !field || !field->type)
return NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

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


int alignment = 0;
int element_size = mono_type_size (t, &alignment);

#if G_BYTE_ORDER == G_LITTLE_ENDIAN
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#if G_BYTE_ORDER == G_LITTLE_ENDIAN
#if G_BYTE_ORDER == G_LITTLE_ENDIAN

Copy link
Member Author

Choose a reason for hiding this comment

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

I have moved the preprocessor directives.

int swizzle = 1;
#else
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

int swizzle = element_size;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Ditto


gpointer data_ptr = (gpointer)mono_field_get_rva (field, swizzle);
const int num_elements = mono_type_size (field->type, &alignment) / element_size;
const int obj_size = MONO_ABI_SIZEOF (MonoObject);

MonoInst* span = mono_compile_create_var (cfg, fsig->ret, OP_LOCAL);
MonoInst* span_addr;
EMIT_NEW_TEMPLOADA (cfg, span_addr, span->inst_c0);

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

} else {
EMIT_NEW_PCONST (cfg, ptr_inst, data_ptr);
}
MONO_EMIT_NEW_STORE_MEMBASE (cfg, OP_STOREP_MEMBASE_REG, span_addr->dreg, field_ref->offset - obj_size, ptr_inst->dreg);

MonoClassField* field_len = mono_class_get_field_from_name_full (span->klass, "_length", NULL);
MONO_EMIT_NEW_STORE_MEMBASE_IMM (cfg, OP_STOREI4_MEMBASE_IMM, span_addr->dreg, field_len->offset - obj_size, num_elements);

EMIT_NEW_TEMPLOAD (cfg, ins, span->inst_c0);

return ins;
} else
return NULL;
Expand Down
3 changes: 3 additions & 0 deletions src/mono/mono/mini/method-to-ir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10820,9 +10820,12 @@ mono_method_to_ir (MonoCompile *cfg, MonoMethod *method, MonoBasicBlock *start_b
} else {
EMIT_NEW_PCONST (cfg, ins, handle);
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

EMIT_NEW_TEMPLOADA (cfg, addr, vtvar->inst_c0);
MONO_EMIT_NEW_STORE_MEMBASE (cfg, OP_STORE_MEMBASE_REG, addr->dreg, 0, ins->dreg);
EMIT_NEW_TEMPLOAD (cfg, ins, vtvar->inst_c0);
ins->inst_c0 = vtvar->inst_c0;
ins->inst_p1 = handle;
}
}

Expand Down