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: Use likelihood-based edge weights #99736

Merged
merged 19 commits into from
Apr 16, 2024

Conversation

amanasifkhalid
Copy link
Member

Part of #93020. Removes FlowEdge::m_edgeWeightMin and FlowEdge::m_edgeWeightMax, and relies on block weights and edge likelihoods to determine edge weights via FlowEdge::getLikelyWeight.

I'm opening this now so the diffs are visible.

@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 Mar 14, 2024
Copy link
Contributor

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

@amanasifkhalid
Copy link
Member Author

amanasifkhalid commented Mar 14, 2024

@AndyAyersMS lots of diffs, but they aren't as big as I expected... There are some late phases where I don't use likely weights due to likelihoods not being propagated yet, so I'll leave this as a draft for now. I'm going to start working on ensuring successor block weights are updated as edges are created/removed/redirected.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Looks good so far.

My PR to finish likelihoods is in, so would be nice to see a more "complete" solution.

{
jmpBlk->bbWeight = BB_ZERO_WEIGHT;
}
jmpBlk->bbWeight = newEdge->getLikelyWeight();
Copy link
Member

Choose a reason for hiding this comment

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

You can use setBBProfileWeight here

@amanasifkhalid amanasifkhalid marked this pull request as ready for review March 15, 2024 15:51
@amanasifkhalid
Copy link
Member Author

@AndyAyersMS thanks for taking a look! I think the diffs aren't as good as they could be due to unreliable block weights, and I'm assuming fixing that will require a process similar to the edge likelihoods work. I'm curious if you're planning on taking this change with potentially subpar diffs for now?

@amanasifkhalid
Copy link
Member Author

@AndyAyersMS updated diffs still show lots of regressions. I suppose we could leave this open and work on improving block weights first to see if that fixes some of the regressions, or we could merge this, and then it'll be easier to determine if profile maintenance work improves diffs locally -- up to you.

@AndyAyersMS
Copy link
Member

Probably worth another update, now that repairs are there?

@amanasifkhalid
Copy link
Member Author

amanasifkhalid commented Apr 12, 2024

Probably worth another update, now that repairs are there?

I was planning on opening a draft PR for running profile repair right before layout (the diffs were quite dramatic locally), though it might make sense to skip a step and try that here, in conjunction with using likelihood-based edge weights during layout. Would you like me to try that here?

@amanasifkhalid
Copy link
Member Author

In the diffs, I'm still seeing about an equal number of size improvements and regressions on x64, and generally more improvements than regressions on arm64 (probably due to the differences in jump sizes between architectures). Looking at the example regressions on Windows x64, it looks like with likelihood-based edge weights, we're doing a better job of moving cold blocks towards the end of the method, such that the block weights/PerfScores are in descending order -- this results in less fall-through, but if these blocks are truly cold, maybe it's worth the tradeoff?

@amanasifkhalid
Copy link
Member Author

SPMI failures look like #101070.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

I think we should go ahead and take this now.

// A takenRatio of 0.50 means taken 50% of the time, not taken 50% of the time
// A takenRatio of 0.90 means taken 90% of the time, not taken 10% of the time
//
double takenCount = edgeToDest->getLikelyWeight();
Copy link
Member

Choose a reason for hiding this comment

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

We should revise this to just use likelihoods, instead of weights? But we can do this as a follow-up.

//
// Returns:
// Suitable phase status
//
PhaseStatus Compiler::fgComputeBlockAndEdgeWeights()
PhaseStatus Compiler::fgComputeBlockWeights()
Copy link
Member

Choose a reason for hiding this comment

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

This phase should eventually go away. We can compute fgCalledCount much earlier now. But let's leave that for follow-up too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you suggesting we compute fgCalledCount in fgIncorporateProfileData? And would we still want to call fgComputeMissingBlockWeights? I tried removing this phase altogether locally and computing fgCalledCount during profile incorporation, and the diffs from this are quite big

Copy link
Member

Choose a reason for hiding this comment

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

More or less -- but both may still be needed for non-pgo cases?

We are going to need to find the right reconciliation between having PGO and not having it. Ideally perhaps we just always think we have it, but I don't know if we want to pay the costs of this in modes like minopt where we probably don't ever look at the data...

@amanasifkhalid
Copy link
Member Author

@AndyAyersMS Thanks for the review!

@amanasifkhalid amanasifkhalid merged commit 92b9dca into dotnet:main Apr 16, 2024
106 of 110 checks passed
@amanasifkhalid amanasifkhalid deleted the edge-weights branch April 16, 2024 17:42
amanasifkhalid added a commit that referenced this pull request Apr 17, 2024
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
Part of dotnet#93020. Removes FlowEdge::m_edgeWeightMin and FlowEdge::m_edgeWeightMax, and relies on block weights and edge likelihoods to determine edge weights via FlowEdge::getLikelyWeight.
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 17, 2024
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