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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

JIT: Remove some fgRenumberBlocks calls #110026

wants to merge 7 commits into from
+7 −57

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
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

amanasifkhalid
amanasifkhalid commented Nov 20, 2024
View reviewed changes
src/coreclr/jit/compiler.cpp Outdated
// 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 amanasifkhalid mentioned this pull request Nov 21, 2024
Merged
amanasifkhalid added a commit that referenced this pull request Nov 21, 2024
@amanasifkhalid
JIT: Replace BlockSet with BitVec, and remove BB epoch (#110034)
7c1f78f 
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@amanasifkhalid amanasifkhalid

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@amanasifkhalid @AndyAyersMS