-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix inline decision reporting + noinline propagation #87115
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6280,12 +6280,6 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, | |
{ | ||
if (!opts.OptEnabled(CLFLG_INLINING)) | ||
{ | ||
/* XXX Mon 8/18/2008 | ||
* This assert is misleading. The caller does not ensure that we have CLFLG_INLINING set before | ||
* calling impMarkInlineCandidate. However, if this assert trips it means that we're an inlinee and | ||
* CLFLG_MINOPT is set. That doesn't make a lot of sense. If you hit this assert, work back and | ||
* figure out why we did not set MAXOPT for this compile. | ||
*/ | ||
assert(!compIsForInlining()); | ||
return; | ||
} | ||
|
@@ -6301,11 +6295,12 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, | |
assert(call->GetInlineCandidatesCount() > 0); | ||
for (uint8_t candidateId = 0; candidateId < call->GetInlineCandidatesCount(); candidateId++) | ||
{ | ||
InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate", true); | ||
InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate for GDV"); | ||
|
||
// Do the actual evaluation | ||
impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, | ||
ilOffset, &inlineResult); | ||
inlineResult.Report(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calls Report explicitly instead of relying on destructor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What went wrong with relying on the destructor? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted to destructor since it raised a question - I don't have a strong opinion about it |
||
|
||
// Ignore non-inlineable candidates | ||
// TODO: Consider keeping them to just devirtualize without inlining, at least for interface | ||
|
@@ -6316,14 +6311,22 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, | |
candidateId--; | ||
} | ||
} | ||
|
||
// None of the candidates made it, make sure the call is no longer marked as "has inline info" | ||
if (call->GetInlineCandidatesCount() == 0) | ||
{ | ||
assert(!call->IsInlineCandidate()); | ||
assert(!call->IsGuardedDevirtualizationCandidate()); | ||
} | ||
} | ||
else | ||
{ | ||
const uint8_t candidatesCount = call->GetInlineCandidatesCount(); | ||
assert(candidatesCount <= 1); | ||
InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate", true); | ||
InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate"); | ||
impMarkInlineCandidateHelper(call, 0, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, ilOffset, | ||
&inlineResult); | ||
inlineResult.Report(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto here -- does the destructor not work for some reason? |
||
} | ||
|
||
// If this call is an inline candidate or is not a guarded devirtualization | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2335,10 +2335,27 @@ bool DiscretionaryPolicy::PropagateNeverToRuntime() const | |
// | ||
switch (m_Observation) | ||
{ | ||
// Not-profitable depends on call-site: | ||
case InlineObservation::CALLEE_NOT_PROFITABLE_INLINE: | ||
return false; | ||
|
||
// If we mark no-returns as noinline we won't be able to recognize them | ||
// as no-returns in future inlines. | ||
case InlineObservation::CALLEE_DOES_NOT_RETURN: | ||
return false; | ||
|
||
// These also depend on call-sites | ||
case InlineObservation::CALLSITE_OVER_BUDGET: | ||
case InlineObservation::CALLSITE_TOO_MANY_LOCALS: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CALLSITE observations shouldn't lead to "never" reports back to the VM. Only CALLEE observations should have this effect. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, removed |
||
return false; | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to revert these changes so I can study the TP impact separately |
||
case InlineObservation::CALLEE_TOO_MUCH_IL: | ||
return false; | ||
|
||
default: | ||
return true; | ||
} | ||
|
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.
Seems like the comment doesn't add a lot of value