JIT: Generalize entry-block parameter register rewriting to all blocks - #133690
jakobbotsch merged 4 commits into
Conversation
Generalize the existing FindInducedParameterRegisterLocals transformation from entry-block-only rewriting in lowering to all-block rewriting during rationalization. Record parameter field reads, stores, and address uses in the existing execution-order visitor rather than adding a separate IR walk. After rationalization, propagate parameter kills through normal and EH successors, including loop backedges. Rewrite field reads only when every reaching path still observes the incoming parameter value, respecting read-before-kill ordering within each block. Reuse the existing extraction logic and parameter-register target mappings. Remove the entry-block discovery and unused reuse helper from lowering, while retaining mappings for independently promoted parameters. Account for the earlier parameter-register targets in async default-value analysis and invalidate recorded uses when rationalization discards their nodes. Physical promotion's eager readbacks remain unchanged; profitability of keeping packed values live across calls is follow-up work. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4c28b3c3-cf6c-4149-8eb1-964831492011
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS This is working towards a better solution to the general problem in #133603. There the issue was that we had an incoming #134048 is a follow-up to make physical promotion defer the readbacks as appropriate. Diffs. Perfscore improvements, but comes with some code size regressions on arm64. Mainly that looks to be cases where we now keep parts of a parameter in registers, but those end up live across a call, so we end up needing a callee save for it, resulting in larger prolog/epilogs. I think it is a reasonable trade-off, and in any case seems like something that should be left up to LSRA. |
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate findings remain unresolved: regression coverage and reachability cost validation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request generalizes parameter-register field rewriting from entry-block-only lowering to CFG-aware rationalization across all blocks.
Changes:
- Tracks parameter reads, stores, and address uses during rationalization.
- Propagates kills through normal, EH, and loop successors.
- Removes obsolete lowering logic and updates promotion and async analysis.
File summaries
| File | Summary |
|---|---|
src/coreclr/jit/rationalize.h |
Adds parameter-use tracking state and helpers. |
src/coreclr/jit/rationalize.cpp |
Implements CFG-aware recording, kill propagation, and field rewriting. Moderate finding (3 votes): add regression coverage for successor/handler reads, loop/backedge kills, and read-before-kill ordering. Moderate finding (1 vote): provide compile-time/throughput measurements or constrain reachability if regressions are measurable. |
src/coreclr/jit/promotion.cpp |
Updates the extraction logic reference. |
src/coreclr/jit/lower.h |
Removes obsolete helper declarations. |
src/coreclr/jit/lower.cpp |
Removes entry-block discovery while retaining promotion mappings. |
src/coreclr/jit/asyncanalysis.cpp |
Treats parameter-register targets as initialized parameters. |
Review details
Suppressed comments (1)
src/coreclr/jit/rationalize.cpp:2638
- The new loop can perform a full CFG reachability traversal once for every parameter that has reads and kills, changing this optimization from an entry-block scan to O(number of parameters × CFG size) work. Please include JIT compile-time/throughput measurements for large methods, or share/constrain the reachability computation if this shows a measurable regression.
for (unsigned lclNum = 0; lclNum < m_compiler->info.compArgsCount; lclNum++)
{
ParameterUses* uses = m_parameterUses[lclNum];
if ((uses == nullptr) || !uses->HasReads)
{
continue;
}
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Implicitly-byref parameters need exclusion, and regression coverage for CFG kill propagation is required.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/coreclr/jit/rationalize.cpp:2625
- This adds a path-sensitive codegen transformation, but no regression test covers the newly supported non-entry blocks or the kill propagation. The existing physical-promotion tests exercise entry/straight-line field reads; add a case with a read in a successor, loop, or EH handler and a store/address on another reaching path, since those cases determine whether this analysis rewrites too much or too little.
void Rationalizer::RewriteParameterUses()
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Address tracking overhead and add focused regression coverage for CFG, EH, loop, and async cases.
Review details
Suppressed comments (2)
src/coreclr/jit/rationalize.cpp:2446
mapParametersis true for every optimized non-OSR method with at least one argument, so this allocates aninfo.compArgsCount-sized tracking array and enables the extra local-use bookkeeping even when no parameter can produce a rewrite. The previous lowering implementation first checked for eligible candidates and returned early; please keep this tracking lazy or gate it on an eligibility scan to avoid adding JIT-time and arena overhead to all optimized argument-taking methods.
bool mapParameters =
m_compiler->opts.OptimizationEnabled() && !m_compiler->opts.IsOSR() && (m_compiler->info.compArgsCount > 0);
#ifdef TARGET_ARM
// The profiler hook on arm32 does not preserve incoming argument registers.
mapParameters &= !m_compiler->compIsProfilerHookNeeded();
src/coreclr/jit/rationalize.cpp:2629
- This introduces path-sensitive rewriting whose correctness depends on read-before-kill ordering, alternate predecessors, EH successors, and loop backedges, but the PR adds no focused JIT regression coverage. Please add tests that exercise those cases (plus the async default-value interaction if applicable), so future changes cannot silently turn a conservative skip into an invalid field extraction.
void Rationalizer::RewriteParameterUses()
{
BitVecTraits traits(m_compiler->fgBBNumMax + 1, m_compiler);
BitVec killedOnEntry = BitVecOps::UninitVal();
bool haveKilledSet = false;
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Generalize the existing FindInducedParameterRegisterLocals transformation from entry-block-only rewriting in lowering to all-block rewriting during rationalization. Record parameter field reads, stores, and address uses in the existing execution-order visitor rather than adding a separate IR walk.
After rationalization, propagate parameter kills through normal and EH successors, including loop backedges. Rewrite field reads only when every reaching path still observes the incoming parameter value, respecting read-before-kill ordering within each block. Reuse the existing extraction logic and parameter-register target mappings.
Remove the entry-block discovery and unused reuse helper from lowering, while retaining mappings for independently promoted parameters. Account for the earlier parameter-register targets in async default-value analysis and invalidate recorded uses when rationalization discards their nodes.
Physical promotion's eager readbacks remain unchanged; profitability of keeping packed values live across calls is follow-up work.