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.

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

JIT: start working on profile consistency #81936

Merged
merged 1 commit into from
Feb 13, 2023

Conversation

AndyAyersMS
Copy link
Member

Dump flow graph before running post-phase checks so the flow graph visualization can be used in conjunction with the profile checker output to spot profile problems.

Various provisional fixes to try and shore up the inevitably inconsistent OSR profile. Add a compensating pseudo-edges to the OSR entry to help explain where the missing flow counts have gone. Relax checking for the OSR entry. Add likelhood when we redirect flow to the OSR entry. Locate the OSR entry and original method entry early so we can special-case them in diagnostics.

Defer marking interesting blocks (say for switch peeling) until we've set edge likelihoods, since (someday) those decisions should use edge likelihoods.

Disable profile checking after profile incorporation. The plan is to walk this disable back incrementally and fix issues in each succesive phase, ultimately checking them all. But currently we still have a lot of check failures right after this first phase.

Contributes to #46885.

Dump flow graph before running post-phase checks so the flow graph
visualization can be used in conjunction with the profile checker output
to spot profile problems.

Various provisional fixes to try and shore up the inevitably inconsistent
OSR profile. Add a compensating pseudo-edges to the OSR entry to help
explain where the missing flow counts have gone. Relax checking for
the OSR entry. Add likelhood when we redirect flow to the OSR entry.
Locate the OSR entry and original method entry early so we can
special-case them in diagnostics.

Defer marking interesting blocks (say for switch peeling) until we've
set edge likelihoods, since (someday) those decisions should use edge
likelihoods.

Disable profile checking after profile incorporation. The plan is to
walk this disable back incrementally and fix issues in each succesive
phase, ultimately checking them all. But currently we still have a lot
of check failures right after this first phase.

Contributes to dotnet#46885.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 10, 2023
@ghost ghost assigned AndyAyersMS Feb 10, 2023
@ghost
Copy link

ghost commented Feb 10, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

Dump flow graph before running post-phase checks so the flow graph visualization can be used in conjunction with the profile checker output to spot profile problems.

Various provisional fixes to try and shore up the inevitably inconsistent OSR profile. Add a compensating pseudo-edges to the OSR entry to help explain where the missing flow counts have gone. Relax checking for the OSR entry. Add likelhood when we redirect flow to the OSR entry. Locate the OSR entry and original method entry early so we can special-case them in diagnostics.

Defer marking interesting blocks (say for switch peeling) until we've set edge likelihoods, since (someday) those decisions should use edge likelihoods.

Disable profile checking after profile incorporation. The plan is to walk this disable back incrementally and fix issues in each succesive phase, ultimately checking them all. But currently we still have a lot of check failures right after this first phase.

Contributes to #46885.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

@BruceForstall PTAL
cc @dotnet/jit-contrib

Should be no diff, no TP impact.

Comment on lines +279 to +280
FlowEdge* const edge = fgAddRefPred(fgFirstBB, block);
edge->setLikelihood(1.0);
Copy link
Member

Choose a reason for hiding this comment

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

Presumably fgAddRefPred could set the likelihood to 1.0 automatically if it's the first (and thus only) pred. Do you want callers to always do that work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not, but I feel like I will need to work through more of the maintenance issues downstream to see what the best patterns should be.

@@ -5234,13 +5234,18 @@ bool Compiler::optInvertWhileLoop(BasicBlock* block)
edgeBlockToAfter->setEdgeWeights(blockToAfterWeight, blockToAfterWeight, bNewCond->bbJumpDest);

#ifdef DEBUG
// Verify profile for the two target blocks is consistent.
// If we're checkig profile data, see if profile for the two target blocks is consistent.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// If we're checkig profile data, see if profile for the two target blocks is consistent.
// If we're checking profile data, see if profile for the two target blocks is consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

I need a spell check for comments. Will fix this in a subsequent PR.

Copy link
Member

Choose a reason for hiding this comment

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

@AndyAyersMS AndyAyersMS merged commit f857468 into dotnet:main Feb 13, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants