-
Notifications
You must be signed in to change notification settings - Fork 5.2k
JIT: Store unique switch successors in BBswtDesc
#117423
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: Store unique switch successors in BBswtDesc
#117423
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors switch and EH-finally successor tracking by introducing BBJumpTable for blocks with N successors and updating BBswtDesc to inherit from it, removing the old BBehfDesc and switch-descriptor map APIs.
- Replace
BBehfDescwithBBJumpTableeverywhere and migrate EH-finally successor handling. - Update
BBswtDescto store both the full case list and a unique successor list viaBBJumpTable. - Remove compiler-level switch-desc map and enable direct unique-successor enumeration through the new API.
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| block.h | Added BBJumpTable, updated switch/EH descriptor types and APIs. |
| switchrecognition.cpp | Updated switch conversion to allocate and initialize BBJumpTable. |
| optimizer.cpp, morph.cpp, lower.cpp, importer.cpp, fg*.cpp, codegencommon.cpp, assertionprop.cpp | Replaced old descriptor field accesses with new GetSuccs/GetCases and API calls. |
Comments suppressed due to low confidence (3)
src/coreclr/jit/block.h:2298
- Add a brief doc comment above
BBJumpTableexplaining its role as a generic descriptor for blocks with N successors, how it differs fromBBswtDesc, and its lifecycle/ownership.
struct BBJumpTable
src/coreclr/jit/block.h:2319
- There are no existing unit tests that exercise the new
BBJumpTable::GetSuccs/RemoveSucc/RemoveDefaultCaseAPIs. Consider adding tests to validate unique-successor enumeration and mutation under both switch and EH-finally scenarios.
FlowEdge** GetSuccs() const
src/coreclr/jit/block.h:1412
- [nitpick] This C-style cast could be replaced with
static_cast<BBJumpTable*>for clarity and safety.
// for (BasicBlock* const bTarget : block->SwitchCases()) ...
|
@dotnet/jit-contrib PTAL. Small diffs. Some components of the JIT, such as our DFS computations, are sensitive to the order in which unique block successors are enumerated. Thus, I tried to preserve the current order of switch successors at each point where we compute them, but because we no longer need to recompute them when flow changes, the ordering can change if the switch's case labels are modified. This seems to rarely make a difference in codegen, so I think we ought to tolerate it. |
|
ping @dotnet/jit-contrib |
|
@AndyAyersMS could you PTAL? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote these up last week but apparently never submitted them.
|
@AndyAyersMS thank you for the review -- I've addressed your feedback |
|
/ba-g build timeouts, #117724 |
Follow-up to #117423. * Collapse `BBSuccList` and `BBSuccEdgeList` into one templatized type, where the template specifies the iterator kind * Remove `BasicBlock::NumSucc(Compiler*)` et al * Have `BasicBlock::NumSucc()` et al deal only with unique successors, now that they're always available * For the occasional case where we have a switch block, and we want to iterate its cases with duplicates, we don't rely on the general successor iteration pattern, anyway * (unrelated) Remove `BasicBlock::bbFallsThrough`
BBehfDesctoBBJumpTable, and repurpose it for describing blocks withNsuccessors. This type allows unique successor access, enumeration, and removal.BBswtDescinherit fromBBJumpTable. ThroughBBswtDesc's updated API surface, we can enumerate unique successors and switch cases.Compiler's API surface for mapping switch blocks to their unique successors.There are numerous points in the JIT where we want to iterate unique successors, but we tolerate iterating duplicates out of fear of needing to construct the switch successor map. We can now iterate unique successors at those sites without needing to precompute anything. I'll do that switchover in a follow-up PR.
This change is regrettably large for one PR, though most of the changes are trivial adjustments to match the new API surface.