Uh oh!
There was an error while loading. Please reload this page.
Fix inline decision reporting + noinline propagation - #87115
Conversation
ghost
commented
Jun 4, 2023
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsIn #86551 I broke reporting of inline decisions, it led to two problems:
The 2nd led to interesting effects - lots of benchmarks improved: #86902 (comment) (some regressed). so I am fixing this back but also - extending the "allow list" in
|
@AndyAyersMS PTAL, we talked about this last week |
| { | ||
| if (!opts.OptEnabled(CLFLG_INLINING)) | ||
| { | ||
| /* XXX Mon 8/18/2008 |
There was a problem hiding this comment.
Seems like the comment doesn't add a lot of value
| // Do the actual evaluation | ||
| impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, | ||
| ilOffset, &inlineResult); | ||
| inlineResult.Report(); |
There was a problem hiding this comment.
Calls Report explicitly instead of relying on destructor
There was a problem hiding this comment.
What went wrong with relying on the destructor?
There was a problem hiding this comment.
Reverted to destructor since it raised a question - I don't have a strong opinion about it
| // These also depend on call-sites | ||
| case InlineObservation::CALLSITE_OVER_BUDGET: | ||
| case InlineObservation::CALLSITE_TOO_MANY_LOCALS: |
There was a problem hiding this comment.
CALLSITE observations shouldn't lead to "never" reports back to the VM.
Only CALLEE observations should have this effect.
| // Do the actual evaluation | ||
| impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, | ||
| ilOffset, &inlineResult); | ||
| inlineResult.Report(); |
There was a problem hiding this comment.
What went wrong with relying on the destructor?
| InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate"); | ||
| impMarkInlineCandidateHelper(call, 0, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, ilOffset, | ||
| &inlineResult); | ||
| inlineResult.Report(); |
There was a problem hiding this comment.
Ditto here -- does the destructor not work for some reason?
| // These thresholds may depend on PGO data (we allow more blocks/bigger IL size | ||
| // for hot methods so we want to make sure we won't bake 'noinline' just because | ||
| // some semi-hot callsite didn't expand these thresholds). | ||
| case InlineObservation::CALLEE_TOO_MANY_BASIC_BLOCKS: |
There was a problem hiding this comment.
I think this will have a detrimental effect on TP as we now will repeatedly analyze even very large methods to see if they are good inline candidates. Surely there must be some size/complexity value that will disqualify a method from ever being inlined.
As an alternative, consider making the call to SetNever conditional on some higher threshold, for policies that handle these observations, eg see ProfilePolicy::NoteInt.
There was a problem hiding this comment.
I decided to revert these changes so I can study the TP impact separately
In #86551 I broke reporting of inline decisions, it led to two problems:
The 2nd led to interesting effects - lots of benchmarks improved: #86902 (comment) (some regressed).
so I am fixing this back but also - extending the "allow list" in
PropagateNeverToRuntimeto not to propagate some specific inline decisions if they might depend on a call-site to avoid regressing other call-sites where context is different.