-
Notifications
You must be signed in to change notification settings - Fork 5k
JIT: remove unnecessary inlining restriction #116773
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
We can still inline methods even if they make generic virtual calls. Fixes dotnet#116740.
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.
Pull Request Overview
This PR removes the special-case restriction that prevented methods making generic virtual calls from being inlined, allowing the JIT to inline those methods.
- Dropped the
IS_GENERIC_VIRTUAL
inline observation so it’s no longer treated as a fatal inlining barrier. - Removed the importer logic that explicitly rejected generic virtual calls during inlining.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/coreclr/jit/inline.def | Removed the IS_GENERIC_VIRTUAL observation entry. |
src/coreclr/jit/importercalls.cpp | Deleted the conditional that forbade inlining generic virtual calls. |
Comments suppressed due to low confidence (1)
src/coreclr/jit/importercalls.cpp:182
- Add unit tests that exercise inlining of methods containing generic virtual calls to verify the new behavior and prevent regressions.
}
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
On the example from #116740, the inner loop is now G_M20355_IG03: ; offs=0x000002, size=0x000A, bbWeight=101.11, PerfScore 454.98, gcrefRegs=0002 {rcx}, byrefRegs=0000 {}, loop=IG03, BB02 [0001], byref, isz
IN0003: 000002 add dword ptr [rcx+0x08], eax
IN0004: 000005 inc eax
IN0005: 000007 cmp eax, 100
IN0006: 00000A jl SHORT G_M20355_IG03 |
@dotnet/jit-contrib PTAL CI SPMI likely won't see anything but missing contexts. Will try and look at a bespoke SPMI diff tomorrow. |
There is another check here that will be hit when the actual GVM is not statically resolvable: runtime/src/coreclr/jit/importercalls.cpp Lines 382 to 388 in f1a4b89
Can it be removed too? |
Related: #112353 |
Seems like it can. |
Results from a local bespoke asp.net SPMI (on this PR). Modest number of base context misses, so not as clean as I'd like, but the trend is clear. Diffs are based on 198,971 contexts (108,879 MinOpts, 90,092 FullOpts). MISSED contexts: base: 219 (0.11%), diff: 0 (0.00%) Overall (+300,358 bytes)
FullOpts (+300,358 bytes)
and for TP (likely overstated a bit due to base misses...)
More details: diff_summary.86.md |
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.
LGTM
/ba-g failure with no log |
We can still inline methods even if they make generic virtual calls.
Fixes #116740.