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: Remove some fgRenumberBlocks calls #110026

Merged
merged 7 commits into from
Nov 25, 2024

Conversation

amanasifkhalid
Copy link
Member

Part of #107749. While we cannot get rid of fgRenumberBlocks altogether yet, this gets us close. The JIT still as a few hard dependencies on bbNums reflecting relative lexical order:

  • If the CFG has loops, optSetBlockWeights will use bbNums for naive loop backedge detection.
  • Loop inversion also uses bbNums in lieu of flow edges to guess at a loop's shape.
  • Loop duplication helpers iterate the loop's blocks in lexical order, which requires ordered bbNums. As such, I've left the fgRenumberBlocks calls in loop cloning/unrolling in-place for now.

There's also a soft dependency on bbNum ordering in fgUpdateFlowGraph that can trigger diffs. These diffs were quite small locally, so I think we can tolerate them for now.

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

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

// Ensure the BB epoch is up to date before running loop hoisting to avoid false positives.
// TODO-bbNum: Either replace this check with a breadcrumb like 'fgModified',
// or relax jump threading's prerequisites.
EnsureBasicBlockEpoch();
Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, we should be able to get rid of RBO's dependency on block epoch altogether. Let me do that first.

amanasifkhalid added a commit that referenced this pull request Nov 21, 2024
Part of #107749. Prerequisite for #110026.

Use postorder number-based BitVecs in RBO and block layout.
Use bbID-based BitVecs in fgIncorporateProfileData. This runs early enough in the JIT frontend such that I would expect bbIDs and bbNums to be 1:1, so I don't expect any TP impact from this change.
Switch descriptor creation still uses bbNums as a key into a BitVec as a workaround for BB epoch invariants -- I'll try switching this over to bbID in a follow-up to evaluate the TP cost of a sparser bitset.
@amanasifkhalid
Copy link
Member Author

cc @dotnet/jit-contrib, @jakobbotsch PTAL. Small diffs. The Instrumented Tier0 diffs stem from the fact that bbNums are used in key computations by EfficientEdgeCountBlockToKey here, so the lack of renumbering is churning the instrumentation schema a bit. The resultant codegen diffs are from the address of the profile data now fitting in 32/64 bits, instead of 64/32 bits. Thanks!

@AndyAyersMS
Copy link
Member

The Instrumented Tier0 diffs stem from the fact that bbNums are used in key computations by EfficientEdgeCountBlockToKey

We can change this to use bbID too. It may break reading in static PGO data for a while but so does changing the values of bbNum. Note when we get around to #44372, instrumented inlinees will need to adjust this value to make it inlinee-relative (as if we'd instrumented the method as root).

@amanasifkhalid
Copy link
Member Author

We can change this to use bbID too.

Do we have anything to gain from switching this over to bbID? Assuming bbNum continues to stick around, it seems like it would be easier to keep using it for the block key since it's already inlinee-relative, right?

@AndyAyersMS
Copy link
Member

We can change this to use bbID too.

Do we have anything to gain from switching this over to bbID? Assuming bbNum continues to stick around, it seems like it would be easier to keep using it for the block key since it's already inlinee-relative, right?

bbID seems more stable, but I suppose as long as we're not ever tempted to add renumbering early, this can stay as is.

@amanasifkhalid amanasifkhalid merged commit 1e6311a into dotnet:main Nov 25, 2024
107 of 108 checks passed
amanasifkhalid added a commit that referenced this pull request Nov 28, 2024
Follow-up to #110026. This required a small refactor of FlowGraphNaturalLoop::VisitLoopBlocksLexical to remove its bbNum checks.
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
…0034)

Part of dotnet#107749. Prerequisite for dotnet#110026.

Use postorder number-based BitVecs in RBO and block layout.
Use bbID-based BitVecs in fgIncorporateProfileData. This runs early enough in the JIT frontend such that I would expect bbIDs and bbNums to be 1:1, so I don't expect any TP impact from this change.
Switch descriptor creation still uses bbNums as a key into a BitVec as a workaround for BB epoch invariants -- I'll try switching this over to bbID in a follow-up to evaluate the TP cost of a sparser bitset.
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
Follow-up to dotnet#110026. This required a small refactor of FlowGraphNaturalLoop::VisitLoopBlocksLexical to remove its bbNum checks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

3 participants