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 fallthrough quirk in fgFindInsertPoint #99783

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

amanasifkhalid
Copy link
Member

Part of #95998.

@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.

@AndyAyersMS
Copy link
Member

Have you looked through diffs yet?

@amanasifkhalid
Copy link
Member Author

I've looked at a few, though I'm thinking of adding the check for "fall through" for BBJ_ALWAYS blocks back in. Diffs are less extreme locally with that restriction. I'll update this soon...

@amanasifkhalid
Copy link
Member Author

Diffs are smaller now, though still pretty big -- fgFindInsertPoint is called by fgNewBBinRegion, which is called in quite a few phases, hence the large churn. Those phases include importation, hence the MinOpts diffs. This change makes it harder for fgFindInsertPoint to decide to break up fallthrough behavior between two blocks by choosing to insert between them; previously, if a BBJ_ALWAYS to the next block did not have BBF_NONE_QUIRK set, we assumed the block did not initially jump to the next block, and flowgraph churn just happened to place it before its successor, so it's ok to break the fallthrough behavior. In my opinion, that's a tenuous conclusion, though perhaps so is the assumption that a BBJ_ALWAYS to the next block will continue to "fall through" (as in fgReorderBlocks won't break them up), and thus we should try to preserve the fallthrough here.

Once we have a new block layout algorithm in place, my hope is we can get rid of all of these attempts to prematurely preserve fallthrough behavior, and trust that block layout will do something reasonable (ideally, we'd be able to get rid of fgFindInsertPoint altogether at that point, and just append blocks to the end of the region as they're created, though maybe that might mess with other flowgraph passes?). For now, I'm not sure if we can reduce churn further if we want to get rid of BBF_NONE_QUIRK.

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.

Can you leave a todo here or a note in #93020 to revisit this after we have improved layout?

@amanasifkhalid
Copy link
Member Author

Sure thing; I added a note to #93020.

@amanasifkhalid amanasifkhalid merged commit 731ac23 into dotnet:main Mar 18, 2024
129 checks passed
@amanasifkhalid amanasifkhalid deleted the fgFindInsertPoint branch March 18, 2024 13:56
@github-actions github-actions bot locked and limited conversation to collaborators Apr 26, 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