-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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.
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
Conversation
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Failures are unrelated. |
|
|
||
| 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 */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Are ref shared methods considered as normal?
- If they are, how could we make a workaround on this assumption?
- And finally, wouldn't this change already break some CI pipelines?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
|
Nice work - I see a 1.9% size reduction on the iOS Sample App with LLVM at : MonoScenarios - Power BI -- (please note - That might not be a publicly available report) |
|
cc @rolfbjarne |
This change improves the AOTed code size by reducing the number of generated methods for generics.
It covers the following cases:
After testing the impact of this change on the HelloWorld app built for
iOSSimulatorthe code size is reduced by~2%App was build with:
and compared between branches
main - Size Aandios-size-reduction - Size Bgiving the following results:Fixes #54850