-
Notifications
You must be signed in to change notification settings - Fork 5.2k
JIT: Boost inlining for more derived returns #113256
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
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.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
| if (sig.retType == CORINFO_TYPE_CLASS) | ||
| { | ||
| exactSigRet = info.compCompHnd->isExactType(sig.retTypeClass); | ||
| } |
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.
We avoid calling VM apis during inlining - this potentially can be slow/trigger type load events for every inline attempt (and we do many of them)
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 meant not this one, but eeGetMethodSig(methodHnd, &methodSig); below
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.
this potentially can be slow/trigger type load events for every inline attempt (and we do many of them)
We are already resolving method tokens for calls while scanning the inlinee, which loads all the type dependencies, so I don't see this to be a problem.
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.
this potentially can be slow/trigger type load events for every inline attempt (and we do many of them)
We are already resolving method tokens for calls while scanning the inlinee, which loads all the type dependencies, so I don't see this to be a problem.
Yes and the initial impl of that caused a massive TP regression as far as I remember, so it's not an excuse to continue calling other VM stuff, if we want to land this, we need to measure/make sure it's cheap.
The main bottleneck in application startup are VM calls triggered by JIT's importer
|
Retrigger a @MihuBot as void returns were not filtered before. |
|
I'm not entirely sure I understand this heuristic, but the multiplier and the JIT-diff is too massive to land as is, e.g. it's 10x bigger than Andy's PR to enable EH inlining. Can you extract the CALLEE_LOOKS_LIKE_WRAPPER part to a separate PR? I think that one makes total sense |
Some diffs seem unexpected to me as well, will investigate later.
Sure. |
|
I think we should resolve the issue here to enable yield devirt instead of aggressively bumping the multiplier in the JIT. |
let's convert it to draft then (it shows up in our system as a stale pr) |
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
Boost inlining for more derived returns.
Example:
Before:
Resulting in virtual calls:
After:
All virtual calls get devirtualized thanks to late devirtualization:
cc @AndyAyersMS