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: Factor SSA's DFS and profile synthesis's loop finding #95251

Merged
merged 4 commits into from
Nov 28, 2023

Conversation

jakobbotsch
Copy link
Member

Factor out SSA's general DFS (that takes EH into account) and encapsulate it in a FlowGraphDfsTree class.

Factor out profile synthesis's loop finding and encapsulate it in a FlowGraphNaturalLoops class. Switch construction of it to use the general DFS instead of the restricted one (that does not account for exceptional flow).

Optimize a few things in the process:

  • Avoid storing loop blocks in a larger than necessary bit vector; store them starting from the loop header's postorder index instead.
  • Provide post-order and reverse post-order visitors for the loop blocks; switch profile synthesis to use this in a place

No diffs are expected. A small amount of diffs are expected when profile synthesis is enabled due to the modelling of exceptional flow and also from handling unreachable predecessors (which would reject some loops as unnatural loops before).

My future plans are to proceed to replace the loop representation of loops with this factored version, removing the lexicality requirement in the process, and hopefully fixing some of our deficiencies.

Factor out SSA's general DFS (that takes EH into account) and
encapsulate it in a `FlowGraphDfsTree` class.

Factor out profile synthesis's loop finding and encapsulate it in a
`FlowGraphNaturalLoops` class. Switch construction of it to use the
general DFS instead of the restricted one (that does not account for
exceptional flow).

Optimize a few things in the process:
* Avoid storing loop blocks in a larger than necessary bit vector; store
  them starting from the loop header's postorder index instead.
* Provide post-order and reverse post-order visitors for the loop
  blocks; switch profile synthesis to use this in a place

No diffs are expected. A small amount of diffs are expected when profile
synthesis is enabled due to the modelling of exceptional flow and also
from handling unreachable predecessors (which would reject some loops as
unnatural loops before).

My future plans are to proceed to replace the loop representation of
loops with this factored version, removing the lexicality requirement in
the process, and hopefully fixing some of our deficiencies.
@ghost ghost assigned jakobbotsch Nov 26, 2023
@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 Nov 26, 2023
@ghost
Copy link

ghost commented Nov 26, 2023

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

Issue Details

Factor out SSA's general DFS (that takes EH into account) and encapsulate it in a FlowGraphDfsTree class.

Factor out profile synthesis's loop finding and encapsulate it in a FlowGraphNaturalLoops class. Switch construction of it to use the general DFS instead of the restricted one (that does not account for exceptional flow).

Optimize a few things in the process:

  • Avoid storing loop blocks in a larger than necessary bit vector; store them starting from the loop header's postorder index instead.
  • Provide post-order and reverse post-order visitors for the loop blocks; switch profile synthesis to use this in a place

No diffs are expected. A small amount of diffs are expected when profile synthesis is enabled due to the modelling of exceptional flow and also from handling unreachable predecessors (which would reject some loops as unnatural loops before).

My future plans are to proceed to replace the loop representation of loops with this factored version, removing the lexicality requirement in the process, and hopefully fixing some of our deficiencies.

Author: jakobbotsch
Assignees: jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone: -

// Find the exit edges
//
loop->VisitLoopBlocks([=](BasicBlock* loopBlock) {
loopBlock->VisitRegularSuccs(comp, [=](BasicBlock* succBlock) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This probably should be VisitAllSuccs -- if you have a loop enclosed in a handler, then every block becomes an exit block. But I think I will look at it separately -- once we start using this elsewhere I think it becomes unlikely that we'll want to compute the exit and entry edges proactively here, and presumably profile synthesis only wants to consider the non EH exit edges as part of assigning probabilities. So at that point I think this will end up refactored a bit.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, makes sense -- for profile purposes we will assume exceptions never happen.

BasicBlock* const loopBlock = worklist.back();
worklist.pop_back();

for (FlowEdge* predEdge = comp->BlockPredsWithEH(loopBlock); predEdge != nullptr;
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is the important one to switch to BlockPredsWithEH to recognize the full SCC of loops containing EH constructs.

@jakobbotsch jakobbotsch marked this pull request as ready for review November 27, 2023 13:33
With more general cycles the loop below that looks at predecessors could
see uninitialized weights, so this is definitely needed.
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @AndyAyersMS @BruceForstall

Diffs. Just some small TP regressions that seem to come down to some different enregistration in the DFS lambda I added.

Diffs with DOTNET_JitSynthesizeCounts=1. Almost all of these diffs are from the change that allows us to now identify natural loops where some blocks have unreachable predecessors; previously we would consider these as unnatural loops. If I quirk those cases to be considered as unnatural, then the diffs look like https://gist.github.com/jakobbotsch/28e8612c775577497aa477fe8eeb172b, and come from loop structures that go through exceptional flow.

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.

LGTM.

// Find the exit edges
//
loop->VisitLoopBlocks([=](BasicBlock* loopBlock) {
loopBlock->VisitRegularSuccs(comp, [=](BasicBlock* succBlock) {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, makes sense -- for profile purposes we will assume exceptions never happen.

@jakobbotsch
Copy link
Member Author

I'm going to merge this, but @BruceForstall please feel free to take a look when you have time and I'll try to address your feedback in a follow-up.

@jakobbotsch jakobbotsch merged commit f106d7e into dotnet:main Nov 28, 2023
129 checks passed
@jakobbotsch jakobbotsch deleted the factor-dfs-loops branch November 28, 2023 11:02
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 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