JIT: Forward sub candidate-set multi-statement lookahead#124843
JIT: Forward sub candidate-set multi-statement lookahead#124843AndyAyersMS wants to merge 2 commits intodotnet:mainfrom
Conversation
Refactor forward substitution to walk blocks first-to-last, maintaining a set of candidate def statements. For each statement, check for last uses of candidate locals and attempt substitution. This naturally handles arbitrary lookahead distance and subsumes the old backtracking logic. Key changes: - ForwardSubCandidate struct tracks def, local, cached flags, and accumulated intermediate effects - Extracted fgForwardSubIsDefCandidate for def-side validation - Added fgForwardSubCanReorderPast for bidirectional effect checking - Added fgForwardSubHasStoreInterferenceWithCandidate for intermediate store conflict detection (includes struct field/parent checks) - Refactored fgForwardSubStatement to accept candidate + use statement - Added backward-compat wrapper for promotion.cpp caller - Capped candidate set at 16 to bound O(N*M) work Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
bot
added
the
area-CodeGen-coreclr
|
@jakobbotsch this is more like what you were suggesting. I haven't deeply reviewed or edited any of it. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR refactors the forward substitution optimization to support multi-statement lookahead. Instead of only checking the immediately following statement, the new implementation maintains a candidate set of def statements and processes each statement to find last uses of candidate locals across arbitrary distances.
Changes:
- Introduced ForwardSubCandidate struct to track def statements with accumulated intermediate effects
- Extracted helper functions for modularity:
fgForwardSubIsDefCandidate(def-side validation),fgForwardSubCanReorderPast(bidirectional effect checking), andfgForwardSubHasStoreInterferenceWithCandidate(store conflict detection) - Refactored fgForwardSubBlock to walk statements first-to-last with candidate set tracking and interference removal
- Added backward-compatibility wrapper for existing promotion.cpp caller
- Capped candidate set at 16 to bound O(N*M) complexity
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/forwardsub.cpp | Complete refactoring of forward substitution logic to support multi-statement lookahead with candidate set tracking, extraction of helper functions, and improved effect accumulation across intermediate statements |
| src/coreclr/jit/compiler.h | Added ForwardSubCandidate forward declaration and new helper function signatures |
| candidate->intermediateFlags |= stmtFlags; | ||
| if ((stmtFlags & GTF_EXCEPT) != 0) | ||
| { | ||
| candidate->intermediateExcepts = ExceptionSetFlags::UnknownException; |
There was a problem hiding this comment.
The exception tracking logic unconditionally sets intermediateExcepts = ExceptionSetFlags::UnknownException using assignment rather than bitwise OR. While this works correctly since the field starts as None and stays UnknownException once set, using |= would be more consistent with how intermediateFlags is accumulated on line 1275 and would make the intent clearer. Consider changing line 1278 to: candidate->intermediateExcepts |= ExceptionSetFlags::UnknownException;
| candidate->intermediateExcepts = ExceptionSetFlags::UnknownException; | |
| candidate->intermediateExcepts |= ExceptionSetFlags::UnknownException; |
| // forward sub candidate. Cap the candidate set size to bound | ||
| // the O(N*M) work in the main loop. | ||
| // | ||
| unsigned const maxCandidates = 16; |
There was a problem hiding this comment.
We should make this a jit config option, and also when we hit capacity we might be better off replacing the "oldest" candidate.
| { | ||
| return false; | ||
| } | ||
| if (((fwdFlags & GTF_EXCEPT) != 0) && ((stmtFlags & GTF_EXCEPT) != 0)) |
There was a problem hiding this comment.
I think we may want to allow similar exceptions to be re-ordered.
Refactor forward substitution to walk blocks first-to-last, maintaining a set of candidate def statements. For each statement, check for last uses of candidate locals and attempt substitution. This naturally handles arbitrary lookahead distance and subsumes the old backtracking logic.
Key changes: