Skip to content
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.

Sign up for GitHub

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

Jump to bottom

JIT: prefer edge counts; keep class profiles even if tossing counts #85406

Merged
merged 1 commit into from
Apr 27, 2023
Merged

JIT: prefer edge counts; keep class profiles even if tossing counts #85406

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 7 additions & 18 deletions src/coreclr/jit/fgprofile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2704,29 +2704,19 @@ PhaseStatus Compiler::fgIncorporateProfileData()

if (fgPgoHaveWeights)
{
// We expect not to have both block and edge counts. We may have other
// forms of profile data even if we do not have any counts.
// If for some reason we have both block and edge counts, prefer the edge counts.
//
// As of 4/6/2023 the following invariant check turns out to no longer hold.
// Tracking issue: https://github.com/dotnet/runtime/issues/84446
//
// assert(!haveBlockCounts || !haveEdgeCounts);

bool dataIsGood = false;

if (haveBlockCounts)
if (haveEdgeCounts)
{
dataIsGood = fgIncorporateBlockCounts();
dataIsGood = fgIncorporateEdgeCounts();
}
else if (haveEdgeCounts)
else if (haveBlockCounts)
Copy link
Contributor

@IDisposable IDisposable Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be safer to also handle the haveBlockCounts path if the dataIsGood returned from fgIncorporateEdgeCounts() was false? That way we would prefer the edge counts, but if they were not incorporable, then we would try using the block counts if we have them...

bool dataIsGood = false;

if (haveEdgeCounts)
{
    dataIsGood = fgIncorporateEdgeCounts();
}
if (!dataIsGood && haveBlockCounts)
{
    dataIsGood = fgIncorporateBlockCounts();
}

or maybe even this?

bool dataIsGood = (haveEdgeCounts && fgIncorporateEdgeCounts()) || 
                  (haveBlockCounts && fgIncorporateBlockCounts());

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a reasonable suggestion, but I really don't expect to see block counts anymore, and especially do not expect to see both block and edge counts for the same method -- #80481 enabled edge counts for all cases.

As noted in #84446, for some unknown reason the StandardOptimization.mibc file that comes from data collected by our internal optimization process (which merges profile data from multiple runs of things) had some cases where a method hand both block and edge counts. We are in the process of updating this profile data (see eg #85193); I should check if that's still the case.

{
dataIsGood = fgIncorporateEdgeCounts();
dataIsGood = fgIncorporateBlockCounts();
}

// Profile incorporation may have tossed out all PGO data if it
// encountered major issues. This is perhaps too drastic. Consider
// at least keeping the class profile data, or perhaps enable full synthesis.
//
// If profile incorporation hit fixable problems, run synthesis in blend mode.
//
if (fgPgoHaveWeights && !dataIsGood)
Expand Down Expand Up @@ -3551,10 +3541,9 @@ void EfficientEdgeCountReconstructor::Propagate()
//
if (m_badcode || m_mismatch || m_failedToConverge || m_allWeightsZero)
{
// Make sure nothing else in the jit looks at the profile data.
// Make sure nothing else in the jit looks at the count profile data.
//
m_comp->fgPgoHaveWeights = false;
m_comp->fgPgoSchema = nullptr;

if (m_badcode)
{
Expand All @@ -3573,7 +3562,7 @@ void EfficientEdgeCountReconstructor::Propagate()
m_comp->fgPgoFailReason = "PGO data available, profile data was all zero";
}

JITDUMP("... discarding profile data: %s\n", m_comp->fgPgoFailReason);
JITDUMP("... discarding profile count data: %s\n", m_comp->fgPgoFailReason);
return;
}

Expand Down