Conversation
- Fix typos (bactrack->backtrack, unalised->unaliased, incomplete sentence) - Add const to EffectsVisitor::GetFlags() - Fix misleading JITDUMP message for address-exposed locals - Move profitability checks earlier for throughput improvement - Add JitForwardSubLookahead config to search N statements ahead for use - Support intermediate statement effect accumulation and store interference Co-authored-by: Copilot <[email protected]>
github-actions
bot
added
the
area-CodeGen-coreclr
There was a problem hiding this comment.
Pull request overview
This PR refactors and enhances the forward substitution optimization in the JIT compiler by introducing multi-statement lookahead capability. Forward substitution attempts to merge a local variable definition with its single use, eliminating the intermediate local. Previously, the optimization only checked the immediately following statement; now it can search ahead multiple statements (configurable via JitForwardSubLookahead, default 10).
Changes:
- Introduces configurable multi-statement lookahead for forward substitution to find uses that are not in the immediate next statement
- Moves profitability checks earlier in the flow to avoid wasted work
- Fixes spelling errors and improves code comments/documentation throughout
- Renames variables from
nextStmttouseStmtfor better clarity in the context of multi-statement lookahead
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/jit/jitconfigvalues.h | Adds JitForwardSubLookahead configuration with default value of 10 to control how many statements ahead to search |
| src/coreclr/jit/forwardsub.cpp | Implements multi-statement lookahead logic with intermediate effect accumulation, moves profitability checks earlier, fixes typos ("backtrack", "unaliased", "exceptions can be thrown"), adds SetIntermediateEffects method, makes GetFlags() const, renames nextStmt to useStmt throughout, and updates documentation |
| unsigned lookahead = max(1u, (unsigned)JitConfig.JitForwardSubLookahead()); | ||
|
|
There was a problem hiding this comment.
JitForwardSubLookahead() returns an int. Casting to unsigned before clamping means negative config values wrap to a huge lookahead, potentially scanning the rest of the block and doing extra gtUpdateStmtSideEffects/walks. Clamp in the signed domain first (e.g., treat values <= 0 as 1) before converting to unsigned.
| unsigned lookahead = max(1u, (unsigned)JitConfig.JitForwardSubLookahead()); | |
| int configLookahead = JitConfig.JitForwardSubLookahead(); | |
| if (configLookahead <= 0) | |
| { | |
| configLookahead = 1; | |
| } | |
| unsigned lookahead = static_cast<unsigned>(configLookahead); |
| if (!found) | ||
| if (useStmt == nullptr) | ||
| { | ||
| JITDUMP(" no next stmt use\n"); |
There was a problem hiding this comment.
The diagnostic message "no next stmt use" is now misleading because the logic can scan multiple statements (up to JitForwardSubLookahead) rather than just the next statement. Consider updating the message to reflect that no eligible use was found within the lookahead range.
| JITDUMP(" no next stmt use\n"); | |
| JITDUMP(" no eligible use within lookahead range\n"); |
|
Seems like this approach has some flaws.. going to try something a bit more general |
1 participant
No description provided.