Skip to content

Conversation

@amanasifkhalid
Copy link
Contributor

Part of #95998.

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 14, 2024
@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.

@AndyAyersMS
Copy link
Member

Have you looked through diffs yet?

@amanasifkhalid
Copy link
Contributor 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
Contributor 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
Contributor Author

Sure thing; I added a note to #93020.

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Mar 26, 2024

@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