-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Enable inlining of shared generics code within same type #38229
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
9645520
to
dd93ffa
Compare
8cbb058
to
3770ae3
Compare
3819aa0
to
d09090e
Compare
Asm diffs
|
Example of code-size regression - inlining tends to increase code size on average:
Baseline
Diff
|
Example of code-size improvement - inlining reduces code size when it enables additional constant-prop:
Baseline:
Diff:
|
The example improvements are exactly the kind I am hoping for 👍 |
@dotnet/crossgen-contrib Any crossgen2 specific legs you would recommend to run on this? @dotnet/jit-contrib Any JIT specific legs you would recommend to run on this? |
src/coreclr/src/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs
Outdated
Show resolved
Hide resolved
src/coreclr/src/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs
Outdated
Show resolved
Hide 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.
Crossgen2 side looks good to me.
I echo Manish's question about the assert - I think that code might be reachable.
Not commenting on the parallel sort thing because I didn't look at that code before.
for crossgen2 would be good to run the |
/azp run runtime-coreclr crossgen2 |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
@mangod9 What is the right way to trigger it? I have tried to trigger it above, but it does not seem to work. |
Hmm.. not sure why that pipeline command fails (probably some config for the pipeline @trylek ?). For now I have manually triggered it via AzDo portal |
I thought (almost) no triggers were enabled, due to issues with accidentally triggering too many builds. Also, #32777 |
@jkotas, you can no longer use any of the pipelines from the AzDo command line. You need to find the pipeline in AzDo and manually start a build. |
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.
Were those diffs from crossgen or PMI?
Might want to run a wider set of diffs (pass -f
to jit-diff) and/or look diffs from both prejitting and jitting.
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'd like you to split out the work to address the static field address issue into a separate PR, and to provide a better description of under what conditions an inlined shared generic lookup will work and where it won't work.
@davidwrighton Done. The updated description is at the top. |
Seems like it would be worthwhile to |
-f crossgen diffs:
|
|
Great to see this merged. 👍
This is quite clear but to be sure, I assume this means this won't work for base class methods for example? E. g. called in a derived class? |
Correct. |
This change allows inlining of generic dictionary lookups when the caller and callee are the same exact type and instantiation. It is a simpler version of #31833.
Motivation for this change is to unblock #37941.
Example: