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: Move targets of hot jumps to create fallthrough post-RPO layout #102927

Merged
merged 9 commits into from
Jun 17, 2024

Conversation

amanasifkhalid
Copy link
Member

@amanasifkhalid amanasifkhalid commented May 31, 2024

After establishing an RPO-based layout, we currently try to move any backward jumps up to their successors, if it is profitable to do so in spite of any fallthrough behavior lost. In #102763, we see many instances where the RPO layout fails to create fallthrough for forward jumps on the hot path, such as in cases where a block is reachable from many predecessors. This work addresses the RPO's limitations by also considering moving the targets of forward jumps (conditional and unconditional) to maximize fallthrough.

cc @dotnet/jit-contrib -- parsing the diffs of this is tedious, so check out `#102763 for some example changes in block layout. Thanks!

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

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

@AndyAyersMS
Copy link
Member

Seems like k-opt is going to need more bake time, so let's focus on getting this merged for now.

@amanasifkhalid
Copy link
Member Author

Seems like k-opt is going to need more bake time, so let's focus on getting this merged for now.

Sounds like a plan; I think this is ready for review. TP cost isn't nothing, but I think we're still in the green relative to the cost of the old layout.

@AndyAyersMS
Copy link
Member

Seems like k-opt is going to need more bake time, so let's focus on getting this merged for now.

Sounds like a plan; I think this is ready for review. TP cost isn't nothing, but I think we're still in the green relative to the cost of the old layout.

There are some substantial code size increases -- is this because we're more aggressively moving cold code now?

@amanasifkhalid
Copy link
Member Author

There are some substantial code size increases -- is this because we're more aggressively moving cold code now?

I think that's part of it -- since the threshold for what counts as cold code is now slightly more liberal, there are probably more instances where we don't move a block's target up, and instead move it to the cold section in fgMoveColdBlocks. Also, if we find a forward jump target that doesn't immediately succeed its predecessor after the RPO layout, it's probably because that target is reachable from many predecessors, so by moving it up to its hottest pred, we are turning existing fallthrough into a backward jump, and potentially increasing the lengths of other jumps to this block (on x86/x64). So code size from branches increases overall, but the hot path loses a branch, so it seems worth it?

@AndyAyersMS
Copy link
Member

So code size from branches increases overall, but the hot path loses a branch, so it seems worth it?

As long as the hot code density improves it should be ok to have somewhat more code overall. Hot/cold splitting would have a similar effect.

@amanasifkhalid
Copy link
Member Author

SPMI failure is timeout.

@amanasifkhalid amanasifkhalid merged commit 6e88cf8 into dotnet:main Jun 17, 2024
105 of 107 checks passed
@amanasifkhalid amanasifkhalid deleted the create-hot-fallthrough branch June 17, 2024 18:10
@LoopedBard3
Copy link
Member

LoopedBard3 commented Jun 20, 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.

3 participants