Skip to content

Do not generate impossible instantiations for constrained generics in Mono AOT #70838

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

Merged
ivanpovazan merged 5 commits into from
Jun 21, 2022
Merged

Do not generate impossible instantiations for constrained generics in Mono AOT #70838

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5e55fa7
Do not generate impossible instantiations for contrained generics
ivanpovazan Jun 16, 2022
dc4dbb9
Removing unused variable
ivanpovazan Jun 16, 2022
1fb385e
Always generate extra methods when new() constraint is specified
ivanpovazan Jun 17, 2022
4c6c194
Fixing the logic for detecting which type parameter constraints requi…
ivanpovazan Jun 18, 2022
a02ceb0
Fixing typos and wording
ivanpovazan Jun 18, 2022
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
50 changes: 48 additions & 2 deletions src/mono/mono/mini/aot-compiler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12307,6 +12307,52 @@ emit_unwind_info_sections_win32 (MonoAotCompile *acfg, const char *function_star
}
#endif

/**
* should_emit_extra_method_for_generics:
* \param method The method to check for generic parameters.
* \param reference_type Specifies which constraint type to look for (TRUE - reference type, FALSE - value type).
*
* Tests whether a generic method or a method of a generic type requires generation of extra methods based on its constraints.
* If a method has at least one generic parameter constrained to a reference type, then REF shared method must be generated.
* On the other hand, if a method has at least one generic parameter constrained to a value type, then GSHAREDVT method must be generated.
* A special case, when extra methods are always generated, is for generic methods of generic types.
*
* Returns: TRUE - extra method should be generated, otherwise FALSE
*/
static gboolean
should_emit_extra_method_for_generics (MonoMethod *method, gboolean reference_type)
{
MonoGenericContainer *gen_container = NULL;
MonoGenericParam *gen_param = NULL;
unsigned int gen_param_count;

if (method->is_generic && mono_class_is_gtd (method->klass)) {
// TODO: This is rarely encountered and would increase the complexity of covering such cases.
// For now always generate extra methods.
return TRUE;
} else if (method->is_generic) {
gen_container = mono_method_get_generic_container (method);
gen_param_count = mono_method_signature_internal (method)->generic_param_count;
} else if (mono_class_is_gtd (method->klass)) {
gen_container = mono_class_get_generic_container (method->klass);
gen_param_count = gen_container->type_argc;
} else {
return FALSE; // Not a generic.
}
g_assert (gen_param_count != 0);

// Inverted logic - for example:
// if we are checking whether REF shared method should be generated (reference_type = TRUE),
// we are looking for at least one constraint which is not a value type constraint.
gboolean should_emit = FALSE;
unsigned int gen_constraint_mask = reference_type ? GENERIC_PARAMETER_ATTRIBUTE_VALUE_TYPE_CONSTRAINT : GENERIC_PARAMETER_ATTRIBUTE_REFERENCE_TYPE_CONSTRAINT;
for (unsigned int i = 0; i < gen_param_count; i++) {
gen_param = mono_generic_container_get_param (gen_container, i);
should_emit |= !(gen_param->info.flags & gen_constraint_mask);
}
return should_emit;
}

static gboolean
should_emit_gsharedvt_method (MonoAotCompile *acfg, MonoMethod *method)
{
Expand All @@ -12318,7 +12364,7 @@ should_emit_gsharedvt_method (MonoAotCompile *acfg, MonoMethod *method)
if (acfg->image == mono_get_corlib () && !strcmp (m_class_get_name (method->klass), "Volatile"))
/* Read<T>/Write<T> are not needed and cause JIT failures */
return FALSE;
return TRUE;
return should_emit_extra_method_for_generics (method, FALSE);
}

static gboolean
Expand Down Expand Up @@ -12368,7 +12414,7 @@ collect_methods (MonoAotCompile *acfg)
}
*/

if (method->is_generic || mono_class_is_gtd (method->klass)) {
if (should_emit_extra_method_for_generics (method, TRUE)) {
/* Compile the ref shared version instead */

@vargaz vargaz Jun 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The aot runtime assumes that the index of 'normal' methods is the same as their token, which means that all normal methods need to be added. They can fail compilation later.

@ivanpovazan ivanpovazan Jun 19, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am not sure if I quite understand this:

  1. Are ref shared methods considered as normal?
  2. If they are, how could we make a workaround on this assumption?
  3. And finally, wouldn't this change already break some CI pipelines?

@vargaz vargaz Jun 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess we add the method below using add_method_with_index () so it will work.

@ivanpovazan ivanpovazan Jun 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Can this then be marked as resolved?

@vargaz vargaz Jun 21, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So what happens here is we add the original generic method, and the JIT fails to compile it, since you can't compile uninflated generic methods. So the ids/indexes will be correct, since we do add these methods, but this looks a bit ugly. You can see this in action by running with
--aot=print-skipped

method = mini_get_shared_method_full (method, SHARE_MODE_NONE, error);
if (!method) {
Expand Down