From c107322dd287e38075ef5278761d196c0c59d965 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 11 Sep 2026 12:11:09 +0200 Subject: [PATCH 1/7] JIT: Generalize entry-block parameter register rewriting to all blocks 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 --- src/coreclr/jit/asyncanalysis.cpp | 6 +- src/coreclr/jit/lower.cpp | 339 +------------------------ src/coreclr/jit/lower.h | 6 +- src/coreclr/jit/promotion.cpp | 2 +- src/coreclr/jit/rationalize.cpp | 395 ++++++++++++++++++++++++++++++ src/coreclr/jit/rationalize.h | 25 ++ 6 files changed, 430 insertions(+), 343 deletions(-) diff --git a/src/coreclr/jit/asyncanalysis.cpp b/src/coreclr/jit/asyncanalysis.cpp index b0e035996758d6..a29053ba0dfa4e 100644 --- a/src/coreclr/jit/asyncanalysis.cpp +++ b/src/coreclr/jit/asyncanalysis.cpp @@ -260,7 +260,7 @@ void DefaultValueAnalysis::ComputePerBlockMutatedVars() // Transfer function: mutatedOut[B] = mutatedIn[B] | mutated[B] // Merge: mutatedIn[B] = union of mutatedOut[pred] for all preds // -// At entry, only parameters and OSR locals are considered mutated. +// At entry, parameters, parameter register targets, and OSR locals are considered mutated. // void DefaultValueAnalysis::ComputeInterBlockDefaultValues() { @@ -271,13 +271,13 @@ void DefaultValueAnalysis::ComputeInterBlockDefaultValues() VarSetOps::AssignNoCopy(m_compiler, m_mutatedVarsIn[i], VarSetOps::MakeEmpty(m_compiler)); } - // Parameters and OSR locals are considered mutated at method entry. + // Parameters, parameter register targets, and OSR locals are non-default at method entry. for (unsigned i = 0; i < m_compiler->lvaTrackedCount; i++) { unsigned lclNum = m_compiler->lvaTrackedToVarNum[i]; LclVarDsc* varDsc = m_compiler->lvaGetDesc(lclNum); - if (varDsc->lvIsParam || varDsc->lvIsOSRLocal) + if (varDsc->lvIsParam || varDsc->lvIsParamRegTarget || varDsc->lvIsOSRLocal) { VarSetOps::AddElemD(m_compiler, m_mutatedVarsIn[m_compiler->fgFirstBB->bbNum], varDsc->lvVarIndex); } diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 65efdbf479e06f..4f930deb4b5b6c 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -9061,8 +9061,11 @@ PhaseStatus Lowering::DoPhase() // void Lowering::MapParameterRegisterLocals() { - m_compiler->m_paramRegLocalMappings = - new (m_compiler, CMK_ABI) ArrayStack(m_compiler->getAllocator(CMK_ABI)); + if (m_compiler->m_paramRegLocalMappings == nullptr) + { + m_compiler->m_paramRegLocalMappings = + new (m_compiler, CMK_ABI) ArrayStack(m_compiler->getAllocator(CMK_ABI)); + } // Create initial mappings for promotions. for (unsigned lclNum = 0; lclNum < m_compiler->info.compArgsCount; lclNum++) @@ -9117,8 +9120,6 @@ void Lowering::MapParameterRegisterLocals() } } - FindInducedParameterRegisterLocals(); - #ifdef DEBUG if (m_compiler->verbose) { @@ -9132,336 +9133,6 @@ void Lowering::MapParameterRegisterLocals() #endif } -//------------------------------------------------------------------------ -// Lowering::FindInducedParameterRegisterLocals: -// Find locals that would be profitable to map from parameter registers, -// based on IR in the initialization block. -// -void Lowering::FindInducedParameterRegisterLocals() -{ -#ifdef TARGET_ARM - // On arm32 the profiler hook does not preserve arg registers, so - // parameters are prespilled and cannot stay enregistered. - if (m_compiler->compIsProfilerHookNeeded()) - { - JITDUMP("Skipping FindInducedParameterRegisterLocals on arm32 with profiler hook\n"); - return; - } -#endif - - // Check if we possibly have any parameters we can induce new register - // locals from. - bool anyCandidates = false; - for (unsigned lclNum = 0; lclNum < m_compiler->info.compArgsCount; lclNum++) - { - LclVarDsc* lcl = m_compiler->lvaGetDesc(lclNum); - if (lcl->lvPromoted || !lcl->lvDoNotEnregister) - { - continue; - } - - const ABIPassingInformation& abiInfo = m_compiler->lvaGetParameterABIInfo(lclNum); - if (!abiInfo.HasAnyRegisterSegment()) - { - continue; - } - - anyCandidates = true; - break; - } - - if (!anyCandidates) - { - return; - } - - bool hasRegisterKill = false; - LocalSet storedToLocals(m_compiler->getAllocator(CMK_ABI)); - // Now look for optimization opportunities in the first block: places where - // we read fields out of struct parameters that can be mapped cleanly. This - // is frequently created by physical promotion. - for (GenTree* node : LIR::AsRange(m_compiler->fgFirstBB)) - { - hasRegisterKill |= node->IsCall(); - - auto visitDefs = [&](GenTreeLclVarCommon* lcl) { - storedToLocals.Emplace(lcl->GetLclNum(), true); - return GenTree::VisitResult::Continue; - }; - - node->VisitLocalDefNodes(m_compiler, visitDefs); - - if (node->OperIs(GT_LCL_ADDR)) - { - // Model these as stored to, since we cannot reason about them in the same way. - storedToLocals.Emplace(node->AsLclVarCommon()->GetLclNum(), true); - continue; - } - - if (!node->OperIs(GT_LCL_FLD)) - { - continue; - } - - GenTreeLclFld* fld = node->AsLclFld(); - if (fld->GetLclNum() >= m_compiler->info.compArgsCount) - { - continue; - } - - LclVarDsc* paramDsc = m_compiler->lvaGetDesc(fld); - if (paramDsc->lvPromoted) - { - // These are complicated to reason about since they may be - // defined/used through their fields, so just skip them. - continue; - } - - if (fld->TypeIs(TYP_STRUCT)) - { - continue; - } - - if (storedToLocals.Lookup(fld->GetLclNum())) - { - // LCL_FLD does not necessarily take the value of the parameter - // anymore. - continue; - } - - const ABIPassingInformation& dataAbiInfo = m_compiler->lvaGetParameterABIInfo(fld->GetLclNum()); - const ABIPassingSegment* regSegment = nullptr; - for (const ABIPassingSegment& segment : dataAbiInfo.Segments()) - { - if (!segment.IsPassedInRegister()) - { - continue; - } - - assert(fld->GetLclOffs() <= m_compiler->lvaLclExactSize(fld->GetLclNum())); - unsigned structAccessedSize = - min(genTypeSize(fld), m_compiler->lvaLclExactSize(fld->GetLclNum()) - fld->GetLclOffs()); - if ((fld->GetLclOffs() < segment.Offset) || - (fld->GetLclOffs() + structAccessedSize > segment.Offset + segment.Size)) - { - continue; - } - - // TODO-CQ: Float -> !float extractions are not supported - // TODO-CQ: Float -> float extractions with non-zero offset is not supported - if (genIsValidFloatReg(segment.GetRegister()) && - (!varTypeUsesFloatReg(fld) || (fld->GetLclOffs() != segment.Offset))) - { - continue; - } - - // Found a register segment this field is contained in - regSegment = &segment; - break; - } - - if (regSegment == nullptr) - { - continue; - } - - JITDUMP("LCL_FLD use [%06u] of unenregisterable parameter is contained in ", Compiler::dspTreeID(fld)); - DBEXEC(VERBOSE, regSegment->Dump()); - JITDUMP("\n"); - - // Now see if we want to introduce a new local for this value, or if we - // can reuse one because this is the source of a store (frequently - // created by physical promotion). - LIR::Use use; - if (!LIR::AsRange(m_compiler->fgFirstBB).TryGetUse(fld, &use)) - { - JITDUMP(" ..but no use was found\n"); - continue; - } - - const ParameterRegisterLocalMapping* existingMapping = - m_compiler->FindParameterRegisterLocalMappingByRegister(regSegment->GetRegister()); - - unsigned remappedLclNum = BAD_VAR_NUM; - if (existingMapping == nullptr) - { - remappedLclNum = m_compiler->lvaGrabTemp(false DEBUGARG( - m_compiler->printfAlloc("V%02u.%s", fld->GetLclNum(), getRegName(regSegment->GetRegister())))); - - // We always use the full width for integer registers even if the - // width is shorter, because various places in the JIT will type - // accesses larger to generate smaller code. - -#ifdef TARGET_WASM - var_types fullWidthType = genActualType(regSegment->GetRegisterType()); -#else - var_types fullWidthType = TYP_I_IMPL; -#endif - var_types registerType = - genIsValidIntReg(regSegment->GetRegister()) ? fullWidthType : regSegment->GetRegisterType(); - if ((registerType == TYP_I_IMPL) && varTypeIsGC(fld)) - { - registerType = fld->TypeGet(); - } - - LclVarDsc* varDsc = m_compiler->lvaGetDesc(remappedLclNum); - varDsc->lvType = genActualType(registerType); - JITDUMP("Created new local V%02u for the mapping\n", remappedLclNum); - - m_compiler->m_paramRegLocalMappings->Emplace(regSegment, remappedLclNum, 0); - varDsc->lvIsParamRegTarget = true; - - JITDUMP("New mapping: "); - DBEXEC(VERBOSE, regSegment->Dump()); - JITDUMP(" -> V%02u\n", remappedLclNum); - } - else - { - remappedLclNum = existingMapping->LclNum; - } - - GenTree* value = m_compiler->gtNewLclVarNode(remappedLclNum); - -#ifdef TARGET_WASM - if (varTypeIsSIMD(value) && !varTypeIsSIMD(fld)) - { - // Unlike native targets, wasm cannot reinterpret a v128 local access as a scalar. - const unsigned laneOffset = fld->GetLclOffs() - regSegment->Offset; - const unsigned scalarSize = genTypeSize(fld); - assert((laneOffset % scalarSize) == 0); - - const unsigned laneIndex = laneOffset / scalarSize; - value = m_compiler->gtNewSimdGetElementNode(fld->TypeGet(), value, - m_compiler->gtNewIconNode(static_cast(laneIndex)), - fld->TypeGet(), genTypeSize(value)); - } - else if (varTypeUsesFloatReg(value)) -#else - if (varTypeUsesFloatReg(value)) -#endif // TARGET_WASM - { - assert(fld->GetLclOffs() == regSegment->Offset); - - value->gtType = fld->TypeGet(); - -#ifdef FEATURE_SIMD - // SIMD12s should be widened. We cannot do that with - // WidenSIMD12IfNecessary as it does not expect to see SIMD12 - // accesses of SIMD16 locals here. - if (value->TypeIs(TYP_SIMD12)) - { - value->gtType = TYP_SIMD16; - } -#endif - } - else - { - var_types registerType = value->TypeGet(); - - if (fld->GetLclOffs() > regSegment->Offset) - { - assert(value->TypeIs(TYP_INT, TYP_LONG)); - GenTree* shiftAmount = m_compiler->gtNewIconNode((fld->GetLclOffs() - regSegment->Offset) * 8, TYP_INT); - value = m_compiler->gtNewOperNode(varTypeIsSmall(fld) && varTypeIsSigned(fld) ? GT_RSH : GT_RSZ, - value->TypeGet(), value, shiftAmount); - } - - // Insert explicit normalization for small types (the LCL_FLD we - // are replacing comes with this normalization). This is only required - // if we didn't get the normalization via a right shift. - if (varTypeIsSmall(fld) && (regSegment->Offset + genTypeSize(fld) != genTypeSize(registerType))) - { - value = m_compiler->gtNewCastNode(TYP_INT, value, false, fld->TypeGet()); - } - - // If the node is still too large then get it to the right size - if (genTypeSize(value) != genTypeSize(genActualType((fld)))) - { - assert(genTypeSize(value) == 8); - assert(genTypeSize(genActualType(fld)) == 4); - - if (value->OperIsScalarLocal()) - { - // We can use lower bits directly - value->gtType = TYP_INT; - } - else - { - value = m_compiler->gtNewCastNode(TYP_INT, value, false, TYP_INT); - } - } - - // Finally insert a bitcast if necessary - if (value->TypeGet() != genActualType(fld)) - { - value = m_compiler->gtNewBitCastNode(genActualType(fld), value); - } - } - - // Now replace the LCL_FLD. - LIR::AsRange(m_compiler->fgFirstBB).InsertAfter(fld, LIR::SeqTree(m_compiler, value)); - use.ReplaceWith(value); - JITDUMP("New user tree range:\n"); - DISPTREERANGE(LIR::AsRange(m_compiler->fgFirstBB), use.User()); - - fld->gtBashToNOP(); - } -} - -//------------------------------------------------------------------------ -// Lowering::TryReuseLocalForParameterAccess: -// Try to figure out if a LCL_FLD that corresponds to a parameter register is -// being stored directly to a LCL_VAR, and in that case whether it would be -// profitable to reuse that local as the parameter register. -// -// Parameters: -// use - The use of the LCL_FLD -// storedToLocals - Map of locals that have had potential definitions to them -// up until the use -// -// Returns: -// The local number to reuse, or BAD_VAR_NUM to create a new local instead. -// -unsigned Lowering::TryReuseLocalForParameterAccess(const LIR::Use& use, const LocalSet& storedToLocals) -{ - GenTree* useNode = use.User(); - - if (!useNode->OperIs(GT_STORE_LCL_VAR)) - { - return BAD_VAR_NUM; - } - - LclVarDsc* destLclDsc = m_compiler->lvaGetDesc(useNode->AsLclVarCommon()); - - if (destLclDsc->lvIsParam || destLclDsc->lvIsParamRegTarget) - { - return BAD_VAR_NUM; - } - - if (destLclDsc->lvIsStructField) - { - return BAD_VAR_NUM; - } - - if (destLclDsc->TypeIs(TYP_STRUCT)) - { - return BAD_VAR_NUM; - } - - if (destLclDsc->lvDoNotEnregister) - { - return BAD_VAR_NUM; - } - - if (storedToLocals.Lookup(useNode->AsLclVarCommon()->GetLclNum())) - { - // Destination may change value before this access - return BAD_VAR_NUM; - } - - return useNode->AsLclVarCommon()->GetLclNum(); -} - #ifdef DEBUG //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 06913e3a8e5b5a..1430446a96ef45 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -137,11 +137,7 @@ class Lowering final : public Phase static bool CheckBlock(Compiler* compiler, BasicBlock* block); #endif // DEBUG - typedef JitHashTable, bool> LocalSet; - - void MapParameterRegisterLocals(); - void FindInducedParameterRegisterLocals(); - unsigned TryReuseLocalForParameterAccess(const LIR::Use& use, const LocalSet& storedToLocals); + void MapParameterRegisterLocals(); void LowerBlock(BasicBlock* block); void AfterLowerBlocks(); diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index 2ecad18d74760f..cbdcdf3b775c46 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -3088,7 +3088,7 @@ bool Promotion::MapsToParameterRegister(Compiler* comp, unsigned lclNum, unsigne for (const ABIPassingSegment& seg : abiInfo.Segments()) { - // This code corresponds to code in Lower::FindInducedParameterRegisterLocals + // This code corresponds to code in Rationalizer::RewriteParameterField. if ((offset < seg.Offset) || (offset + genTypeSize(accessType) > seg.Offset + seg.Size)) { continue; diff --git a/src/coreclr/jit/rationalize.cpp b/src/coreclr/jit/rationalize.cpp index d445a303cd208e..78c376e6313614 100644 --- a/src/coreclr/jit/rationalize.cpp +++ b/src/coreclr/jit/rationalize.cpp @@ -2241,6 +2241,7 @@ Compiler::fgWalkResult Rationalizer::RewriteNode(GenTree** useEdge, Compiler::Ge // and should not violate tree order. assert(isClosed); + ForgetParameterUses(lhsRange); BlockRange().Delete(m_compiler, m_block, std::move(lhsRange)); } else if (op1->IsValue()) @@ -2270,6 +2271,7 @@ Compiler::fgWalkResult Rationalizer::RewriteNode(GenTree** useEdge, Compiler::Ge // LIR and should not violate tree order. assert(isClosed); + ForgetParameterUses(rhsRange); BlockRange().Delete(m_compiler, m_block, std::move(rhsRange)); } else @@ -2343,6 +2345,7 @@ Compiler::fgWalkResult Rationalizer::RewriteNode(GenTree** useEdge, Compiler::Ge { if (use.IsDummyUse()) { + ForgetParameterUses(LIR::ReadOnlyRange(node, node)); BlockRange().Remove(node); } else @@ -2405,6 +2408,10 @@ Compiler::fgWalkResult Rationalizer::RationalizeVisitor::PreOrderVisit(GenTree** // Rewrite HIR nodes into LIR nodes. Compiler::fgWalkResult Rationalizer::RationalizeVisitor::PostOrderVisit(GenTree** use, GenTree* user) { + if ((user != nullptr) || !(*use)->OperIsLocalRead()) + { + m_rationalizer.RecordParameterUse(*use); + } return m_rationalizer.RewriteNode(use, this->m_ancestors); } @@ -2418,6 +2425,17 @@ PhaseStatus Rationalizer::DoPhase() { DBEXEC(TRUE, SanityCheck()); + 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(); +#endif + if (mapParameters) + { + m_parameterUses = new (m_compiler, CMK_ABI) ParameterUses* [m_compiler->info.compArgsCount] {}; + } + m_compiler->compCurBB = nullptr; m_compiler->fgOrder = Compiler::FGOrderLinear; @@ -2475,5 +2493,382 @@ PhaseStatus Rationalizer::DoPhase() m_compiler->compRationalIRForm = true; + if (mapParameters) + { + RewriteParameterUses(); + } + return PhaseStatus::MODIFIED_EVERYTHING; } + +//------------------------------------------------------------------------ +// RecordParameterUse: +// Save a read or kill of a register-passed parameter in execution order. +// +// Arguments: +// node - The node visited by rationalization. +// +void Rationalizer::RecordParameterUse(GenTree* node) +{ + if ((m_parameterUses == nullptr) || !node->OperIs(GT_LCL_FLD, GT_STORE_LCL_VAR, GT_STORE_LCL_FLD, GT_LCL_ADDR)) + { + return; + } + + GenTreeLclVarCommon* lcl = node->AsLclVarCommon(); + unsigned lclNum = lcl->GetLclNum(); + if (lclNum >= m_compiler->info.compArgsCount) + { + return; + } + + LclVarDsc* param = m_compiler->lvaGetDesc(lclNum); + if (param->lvPromoted || (!param->TypeIs(TYP_STRUCT) && !param->lvDoNotEnregister) || + !m_compiler->lvaGetParameterABIInfo(lclNum).HasAnyRegisterSegment()) + { + return; + } + + if (node->OperIs(GT_LCL_FLD) && node->TypeIs(TYP_STRUCT)) + { + return; + } + + ParameterUses*& uses = m_parameterUses[lclNum]; + if (uses == nullptr) + { + uses = new (m_compiler, CMK_ABI) ParameterUses(m_compiler->getAllocator(CMK_ABI)); + } + + uses->Uses.Push(ParameterUse{lcl, m_block}); + uses->HasKills |= !node->OperIs(GT_LCL_FLD); + uses->HasReads |= node->OperIs(GT_LCL_FLD); +} + +//------------------------------------------------------------------------ +// ForgetParameterUses: +// Invalidate recorded local uses before removing a discarded subtree. +// +// Arguments: +// range - The discarded subtree. +// +void Rationalizer::ForgetParameterUses(const LIR::ReadOnlyRange& range) +{ + if (m_parameterUses == nullptr) + { + return; + } + + for (GenTree* node : range) + { + if (!node->OperIs(GT_LCL_FLD, GT_LCL_ADDR) || + (node->AsLclVarCommon()->GetLclNum() >= m_compiler->info.compArgsCount)) + { + continue; + } + + ParameterUses* uses = m_parameterUses[node->AsLclVarCommon()->GetLclNum()]; + if (uses != nullptr) + { + for (ParameterUse& use : uses->Uses.TopDownOrder()) + { + if (use.Node == node) + { + use.Node = nullptr; + break; + } + } + } + } +} + +//------------------------------------------------------------------------ +// RewriteParameterUses: +// Replace field reads that must still observe the incoming parameter value. +// +void Rationalizer::RewriteParameterUses() +{ + BitVecTraits traits(m_compiler->fgBBNumMax + 1, m_compiler); + BitVec killedOnEntry = BitVecOps::UninitVal(); + bool haveKilledSet = false; + ArrayStack worklist(m_compiler->getAllocator(CMK_ABI)); + + for (unsigned lclNum = 0; lclNum < m_compiler->info.compArgsCount; lclNum++) + { + ParameterUses* uses = m_parameterUses[lclNum]; + if ((uses == nullptr) || !uses->HasReads) + { + continue; + } + + if (uses->HasKills) + { + if (!haveKilledSet) + { + killedOnEntry = BitVecOps::MakeEmpty(&traits); + haveKilledSet = true; + } + else + { + BitVecOps::ClearD(&traits, killedOnEntry); + } + + auto queueSuccessor = [&](BasicBlock* successor) { + if (BitVecOps::TryAddElemD(&traits, killedOnEntry, successor->bbNum)) + { + worklist.Push(successor); + } + return BasicBlockVisit::Continue; + }; + + // A kill on any reaching path prevents using the incoming value. Include + // exceptional flow and backedges, even backedges into a block containing a kill. + BasicBlock* lastKillBlock = nullptr; + for (const ParameterUse& use : uses->Uses.BottomUpOrder()) + { + if ((use.Node != nullptr) && !use.Node->OperIs(GT_LCL_FLD) && (use.Block != lastKillBlock)) + { + use.Block->VisitAllSuccs(m_compiler, queueSuccessor); + lastKillBlock = use.Block; + } + } + + while (!worklist.Empty()) + { + worklist.Pop()->VisitAllSuccs(m_compiler, queueSuccessor); + } + } + + BasicBlock* currentBlock = nullptr; + bool killed = false; + for (const ParameterUse& use : uses->Uses.BottomUpOrder()) + { + if (use.Node == nullptr) + { + continue; + } + + if (use.Block != currentBlock) + { + currentBlock = use.Block; + killed = uses->HasKills && BitVecOps::IsMember(&traits, killedOnEntry, currentBlock->bbNum); + } + + if (!use.Node->OperIs(GT_LCL_FLD)) + { + killed = true; + } + else if (!killed) + { + RewriteParameterField(currentBlock, use.Node->AsLclFld()); + } + } + } +} + +//------------------------------------------------------------------------ +// RewriteParameterField: +// Extract a field from a local initialized with its incoming parameter register. +// +// Arguments: +// block - Block containing the field read. +// fld - Field read proven to observe the incoming parameter value. +// +void Rationalizer::RewriteParameterField(BasicBlock* block, GenTreeLclFld* fld) +{ + m_compiler->compCurBB = block; + const ABIPassingInformation& dataAbiInfo = m_compiler->lvaGetParameterABIInfo(fld->GetLclNum()); + const ABIPassingSegment* regSegment = nullptr; + for (const ABIPassingSegment& segment : dataAbiInfo.Segments()) + { + if (!segment.IsPassedInRegister()) + { + continue; + } + + assert(fld->GetLclOffs() <= m_compiler->lvaLclExactSize(fld->GetLclNum())); + unsigned structAccessedSize = + min(genTypeSize(fld), m_compiler->lvaLclExactSize(fld->GetLclNum()) - fld->GetLclOffs()); + if ((fld->GetLclOffs() < segment.Offset) || + (fld->GetLclOffs() + structAccessedSize > segment.Offset + segment.Size)) + { + continue; + } + + // TODO-CQ: Float -> !float extractions are not supported + // TODO-CQ: Float -> float extractions with non-zero offset is not supported + if (genIsValidFloatReg(segment.GetRegister()) && + (!varTypeUsesFloatReg(fld) || (fld->GetLclOffs() != segment.Offset))) + { + continue; + } + + // Found a register segment this field is contained in + regSegment = &segment; + break; + } + + if (regSegment == nullptr) + { + return; + } + + LclVarDsc* param = m_compiler->lvaGetDesc(fld); + var_types segmentType = regSegment->GetRegisterType(param->TypeIs(TYP_STRUCT) ? param->GetLayout() : nullptr); + if ((varTypeIsGC(segmentType) || varTypeIsGC(fld)) && (segmentType != fld->TypeGet())) + { + // The register local must retain the incoming register's GC reporting type. + return; + } + + JITDUMP("LCL_FLD use [%06u] in " FMT_BB " of parameter V%02u is contained in ", Compiler::dspTreeID(fld), + block->bbNum, fld->GetLclNum()); + DBEXEC(VERBOSE, regSegment->Dump()); + JITDUMP("\n"); + + // Find the final LIR use after all statements have been rationalized. + LIR::Use use; + if (!LIR::AsRange(block).TryGetUse(fld, &use)) + { + JITDUMP(" ..but no use was found\n"); + return; + } + + if (m_compiler->m_paramRegLocalMappings == nullptr) + { + m_compiler->m_paramRegLocalMappings = + new (m_compiler, CMK_ABI) ArrayStack(m_compiler->getAllocator(CMK_ABI)); + } + + const ParameterRegisterLocalMapping* existingMapping = + m_compiler->FindParameterRegisterLocalMappingByRegister(regSegment->GetRegister()); + + unsigned remappedLclNum = BAD_VAR_NUM; + if (existingMapping == nullptr) + { + if (!param->lvDoNotEnregister) + { + m_compiler->lvaSetVarDoNotEnregister(fld->GetLclNum() DEBUGARG(DoNotEnregisterReason::LocalField)); + } + + remappedLclNum = m_compiler->lvaGrabTemp(false DEBUGARG( + m_compiler->printfAlloc("V%02u.%s", fld->GetLclNum(), getRegName(regSegment->GetRegister())))); + + // We always use the full width for integer registers even if the + // width is shorter, because various places in the JIT will type + // accesses larger to generate smaller code. + +#ifdef TARGET_WASM + var_types fullWidthType = genActualType(regSegment->GetRegisterType()); +#else + var_types fullWidthType = TYP_I_IMPL; +#endif + var_types registerType = + genIsValidIntReg(regSegment->GetRegister()) ? fullWidthType : regSegment->GetRegisterType(); + if ((registerType == TYP_I_IMPL) && varTypeIsGC(fld)) + { + registerType = fld->TypeGet(); + } + + LclVarDsc* varDsc = m_compiler->lvaGetDesc(remappedLclNum); + varDsc->lvType = genActualType(registerType); + JITDUMP("Created new local V%02u for the mapping\n", remappedLclNum); + + m_compiler->m_paramRegLocalMappings->Emplace(regSegment, remappedLclNum, 0); + varDsc->lvIsParamRegTarget = true; + + JITDUMP("New mapping: "); + DBEXEC(VERBOSE, regSegment->Dump()); + JITDUMP(" -> V%02u\n", remappedLclNum); + } + else + { + remappedLclNum = existingMapping->LclNum; + } + + GenTree* value = m_compiler->gtNewLclVarNode(remappedLclNum); + +#ifdef TARGET_WASM + if (varTypeIsSIMD(value) && !varTypeIsSIMD(fld)) + { + // Unlike native targets, wasm cannot reinterpret a v128 local access as a scalar. + const unsigned laneOffset = fld->GetLclOffs() - regSegment->Offset; + const unsigned scalarSize = genTypeSize(fld); + assert((laneOffset % scalarSize) == 0); + + const unsigned laneIndex = laneOffset / scalarSize; + value = m_compiler->gtNewSimdGetElementNode(fld->TypeGet(), value, + m_compiler->gtNewIconNode(static_cast(laneIndex)), + fld->TypeGet(), genTypeSize(value)); + } + else if (varTypeUsesFloatReg(value)) +#else + if (varTypeUsesFloatReg(value)) +#endif // TARGET_WASM + { + assert(fld->GetLclOffs() == regSegment->Offset); + + value->gtType = fld->TypeGet(); + +#ifdef FEATURE_SIMD + // SIMD12s should be widened. We cannot do that with + // WidenSIMD12IfNecessary as it does not expect to see SIMD12 + // accesses of SIMD16 locals here. + if (value->TypeIs(TYP_SIMD12)) + { + value->gtType = TYP_SIMD16; + } +#endif + } + else + { + var_types registerType = value->TypeGet(); + + if (fld->GetLclOffs() > regSegment->Offset) + { + assert(value->TypeIs(TYP_INT, TYP_LONG)); + GenTree* shiftAmount = m_compiler->gtNewIconNode((fld->GetLclOffs() - regSegment->Offset) * 8, TYP_INT); + value = m_compiler->gtNewOperNode(varTypeIsSmall(fld) && varTypeIsSigned(fld) ? GT_RSH : GT_RSZ, + value->TypeGet(), value, shiftAmount); + } + + // Insert explicit normalization for small types (the LCL_FLD we + // are replacing comes with this normalization). This is only required + // if we didn't get the normalization via a right shift. + if (varTypeIsSmall(fld) && (regSegment->Offset + genTypeSize(fld) != genTypeSize(registerType))) + { + value = m_compiler->gtNewCastNode(TYP_INT, value, false, fld->TypeGet()); + } + + // If the node is still too large then get it to the right size + if (genTypeSize(value) != genTypeSize(genActualType((fld)))) + { + assert(genTypeSize(value) == 8); + assert(genTypeSize(genActualType(fld)) == 4); + + if (value->OperIsScalarLocal()) + { + // We can use lower bits directly + value->gtType = TYP_INT; + } + else + { + value = m_compiler->gtNewCastNode(TYP_INT, value, false, TYP_INT); + } + } + + // Finally insert a bitcast if necessary + if (value->TypeGet() != genActualType(fld)) + { + value = m_compiler->gtNewBitCastNode(genActualType(fld), value); + } + } + + // Now replace the LCL_FLD. + LIR::AsRange(block).InsertAfter(fld, LIR::SeqTree(m_compiler, value)); + use.ReplaceWith(value); + JITDUMP("New user tree range:\n"); + DISPTREERANGE(LIR::AsRange(block), use.User()); + + LIR::AsRange(block).Remove(fld); +} diff --git a/src/coreclr/jit/rationalize.h b/src/coreclr/jit/rationalize.h index 06674cc1f7ca94..d90221785250da 100644 --- a/src/coreclr/jit/rationalize.h +++ b/src/coreclr/jit/rationalize.h @@ -13,6 +13,26 @@ class Rationalizer final : public Phase BasicBlock* m_block; Statement* m_statement; + struct ParameterUse + { + GenTreeLclVarCommon* Node; + BasicBlock* Block; + }; + + struct ParameterUses + { + ArrayStack Uses; + bool HasKills = false; + bool HasReads = false; + + ParameterUses(CompAllocator allocator) + : Uses(allocator) + { + } + }; + + ParameterUses** m_parameterUses = nullptr; + public: Rationalizer(Compiler* comp); @@ -30,6 +50,11 @@ class Rationalizer final : public Phase virtual PhaseStatus DoPhase() override; private: + void RecordParameterUse(GenTree* node); + void ForgetParameterUses(const LIR::ReadOnlyRange& range); + void RewriteParameterUses(); + void RewriteParameterField(BasicBlock* block, GenTreeLclFld* field); + inline LIR::Range& BlockRange() const { return LIR::AsRange(m_block); From f12447dcbfc0e25abde43644fb9fb4d14557a7b1 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 16 Sep 2026 11:39:04 +0200 Subject: [PATCH 2/7] JIT: Defer physical promotion readbacks across basic blocks Visit blocks in reverse postorder and track pending field readbacks at block exits instead of eagerly initializing parameter replacements or materializing every pending readback at each block boundary. Keep replacement locals current by default. Carry the special pending state into a successor only when all reachable predecessors agree. At mixed joins, materialize readbacks on the pending predecessors rather than loading a potentially stale struct home in the successor. Require already-read-back locals at loop/backedge targets and handler entries. Retain regular-successor traversal and explicit EH barriers, including the existing materialization before throwing operations. Readbacks otherwise occur at uses, allowing generalized rationalization to extract parameter fields in the blocks that actually need them. This changes readback placement, not promotion selection or its costing. It avoids unnecessary eager extractions on early-return paths such as Guid.CompareTo while leaving broader profitability tuning as follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4c28b3c3-cf6c-4149-8eb1-964831492011 --- src/coreclr/jit/promotion.cpp | 212 ++++++++++++++++++++++++---------- src/coreclr/jit/promotion.h | 31 +++-- 2 files changed, 170 insertions(+), 73 deletions(-) diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index cbdcdf3b775c46..18464dd4c13a8f 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -1663,11 +1663,57 @@ GenTree* Promotion::CreateReadBack(Compiler* compiler, unsigned structLclNum, co return store; } +//------------------------------------------------------------------------ +// ReplaceVisitor: +// Prepare state for propagating pending readbacks in reverse postorder. +// +// Parameters: +// prom - Promotion phase. +// aggregates - Promoted aggregates. +// liveness - Liveness of the replacements. +// dfsTree - Traversal order, including exceptional flow. +// +ReplaceVisitor::ReplaceVisitor(Promotion* prom, + AggregateInfoMap& aggregates, + PromotionLiveness* liveness, + FlowGraphDfsTree* dfsTree) + : GenTreeVisitor(prom->m_compiler) + , m_promotion(prom) + , m_aggregates(aggregates) + , m_liveness(liveness) + , m_dfsTree(dfsTree) +{ + unsigned index = 0; + for (AggregateInfo* agg : m_aggregates) + { + for (Replacement& rep : agg->Replacements) + { + rep.ReadBackIndex = index++; + } + } + + m_readBackTraits = new (m_compiler, CMK_Promotion) BitVecTraits(index, m_compiler); + m_blockStates = new (m_compiler, CMK_Promotion) BlockState[m_compiler->fgBBNumMax + 1]{}; + + for (unsigned i = 0; i < dfsTree->GetPostOrderCount(); i++) + { + BasicBlock* block = dfsTree->GetPostOrder(i); + m_blockStates[block->bbNum].RequiresAlreadyReadBackOnEntry |= m_compiler->bbIsHandlerBeg(block); + block->VisitRegularSuccs(m_compiler, [&](BasicBlock* succ) { + if (succ->bbPostorderNum >= block->bbPostorderNum) + { + // This includes irreducible backedge targets: their incoming state + // must be settled before visiting predecessors later in RPO. + m_blockStates[succ->bbNum].RequiresAlreadyReadBackOnEntry = true; + } + return BasicBlockVisit::Continue; + }); + } +} + //------------------------------------------------------------------------ // StartBlock: -// Handle reaching the end of the currently started block by preparing -// internal state for upcoming basic blocks, and inserting any necessary -// readbacks. +// Reconcile predecessor states and restore pending readbacks for this block. // // Parameters: // block - The block @@ -1680,8 +1726,7 @@ Statement* ReplaceVisitor::StartBlock(BasicBlock* block) m_currentBlock = block; #ifdef DEBUG - // At the start of every block we expect all replacements to be in their - // local home. + // Descriptors are reset between visits; restore this block's pending state below. for (AggregateInfo* agg : m_aggregates) { for (Replacement& rep : agg->Replacements) @@ -1694,82 +1739,115 @@ Statement* ReplaceVisitor::StartBlock(BasicBlock* block) assert(m_numPendingReadBacks == 0); #endif - // OSR locals and parameters may need an initial read back, which we mark - // when we start the initial BB. - if (block != m_compiler->fgFirstBB) - { - return block->firstStmt(); - } - - Statement* lastInsertedStmt = nullptr; - for (AggregateInfo* agg : m_aggregates) { LclVarDsc* dsc = m_compiler->lvaGetDesc(agg->LclNum); - if (!dsc->lvIsParam && !dsc->lvIsOSRLocal) - { - continue; - } - - JITDUMP("Processing fields of %s V%02u in entry BB " FMT_BB "\n", dsc->lvIsParam ? "parameter" : "OSR-local", - agg->LclNum, block->bbNum); - for (size_t i = 0; i < agg->Replacements.size(); i++) { Replacement& rep = agg->Replacements[i]; - ClearNeedsWriteBack(rep); if (!m_liveness->IsReplacementLiveIn(block, agg->LclNum, (unsigned)i)) { - JITDUMP(" V%02u (%s) ignored because it is not live-in to entry BB\n", rep.LclNum, rep.Description); continue; } - if (!dsc->lvIsParam || - !Promotion::MapsToParameterRegister(m_compiler, agg->LclNum, rep.Offset, rep.AccessType)) + bool pending = false; + if (block == m_compiler->fgFirstBB) { - SetNeedsReadBack(rep); - JITDUMP(" V%02u (%s) marked as needing read back\n", rep.LclNum, rep.Description); - continue; + pending = dsc->lvIsParam || dsc->lvIsOSRLocal; } + else if (!m_blockStates[block->bbNum].RequiresAlreadyReadBackOnEntry) + { + bool hasPred = false; + pending = true; + for (FlowEdge* edge : block->PredEdges()) + { + BasicBlock* pred = edge->getSourceBlock(); + if (!m_dfsTree->Contains(pred)) + { + continue; + } - // Insert read backs of parameters mapping to registers eagerly to - // set the backend up for recognizing these as register accesses. - GenTree* readBack = Promotion::CreateReadBack(m_compiler, agg->LclNum, rep); - Statement* stmt = m_compiler->fgNewStmtFromTree(readBack); - JITDUMP(" V%02u (%s) is read back eagerly because it is a register parameter\n", rep.LclNum, - rep.Description); - DISPSTMT(stmt); - if (lastInsertedStmt == nullptr) + BlockState& state = m_blockStates[pred->bbNum]; + assert(state.Processed); + hasPred = true; + pending &= BitVecOps::IsMember(m_readBackTraits, state.PendingReadBacks, rep.ReadBackIndex); + } + pending &= hasPred; + } + + if (pending) { - m_compiler->fgInsertStmtAtBeg(block, stmt); + ClearNeedsWriteBack(rep); + SetNeedsReadBack(rep); } else { - m_compiler->fgInsertStmtAfter(block, lastInsertedStmt, stmt); + // At a mixed join, read back only on predecessors whose struct is + // current. Loading unconditionally here could read stale fields + // from paths that have updated the replacement instead. + for (FlowEdge* edge : block->PredEdges()) + { + BasicBlock* pred = edge->getSourceBlock(); + if (!m_dfsTree->Contains(pred)) + { + continue; + } + + BlockState& state = m_blockStates[pred->bbNum]; + if (state.Processed && + BitVecOps::IsMember(m_readBackTraits, state.PendingReadBacks, rep.ReadBackIndex)) + { + InsertReadBackAtEnd(pred, agg->LclNum, rep); + BitVecOps::RemoveElemD(m_readBackTraits, state.PendingReadBacks, rep.ReadBackIndex); + } + } } - lastInsertedStmt = stmt; } } - // Skip all the eager read-backs if any were inserted. - return lastInsertedStmt == nullptr ? block->firstStmt() : lastInsertedStmt->GetNextStmt(); + return block->firstStmt(); +} + +//------------------------------------------------------------------------ +// InsertReadBackAtEnd: +// Materialize a pending replacement before leaving a block. +// +// Parameters: +// block - Block in which the struct contains the current value. +// structLclNum - Struct local. +// rep - Replacement to initialize. +// +void ReplaceVisitor::InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNum, const Replacement& rep) +{ + JITDUMP("Reading back V%02u.[%03u..%03u) -> V%02u near the end of " FMT_BB "\n", structLclNum, rep.Offset, + rep.Offset + genTypeSize(rep.AccessType), rep.LclNum, block->bbNum); + + GenTree* readBack = Promotion::CreateReadBack(m_compiler, structLclNum, rep); + Statement* stmt = m_compiler->fgNewStmtFromTree(readBack); + m_compiler->fgInsertStmtNearEnd(block, stmt); } //------------------------------------------------------------------------ // EndBlock: -// Handle reaching the end of the currently started block by preparing -// internal state for upcoming basic blocks, and inserting any necessary -// readbacks. +// Save pending readbacks for successors, materializing them at loop/EH boundaries. // // Remarks: -// We currently expect all fields to be most up-to-date in their field locals -// at the beginning of every basic block. That means all replacements should -// have Replacement::NeedsReadBack == false and Replacement::NeedsWriteBack -// == true at the beginning of every block. This function makes it so that is -// the case. +// Field descriptors are reset between visits; the saved state determines +// which replacements StartBlock can continue to leave in their struct homes. // void ReplaceVisitor::EndBlock() { + bool materialize = m_currentBlock->HasPotentialEHSuccs(m_compiler) || + m_currentBlock->KindIs(BBJ_CALLFINALLY, BBJ_EHFINALLYRET, BBJ_EHFILTERRET, BBJ_EHCATCHRET); + m_currentBlock->VisitRegularSuccs(m_compiler, [&](BasicBlock* succ) { + materialize |= m_blockStates[succ->bbNum].RequiresAlreadyReadBackOnEntry; + return BasicBlockVisit::Continue; + }); + + BlockState& state = m_blockStates[m_currentBlock->bbNum]; + state.PendingReadBacks = BitVecOps::MakeEmpty(m_readBackTraits); + state.Processed = true; + for (AggregateInfo* agg : m_aggregates) { for (size_t i = 0; i < agg->Replacements.size(); i++) @@ -1780,14 +1858,14 @@ void ReplaceVisitor::EndBlock() { if (m_liveness->IsReplacementLiveOut(m_currentBlock, agg->LclNum, (unsigned)i)) { - JITDUMP("Reading back replacement V%02u.[%03u..%03u) -> V%02u near the end of " FMT_BB ":\n", - agg->LclNum, rep.Offset, rep.Offset + genTypeSize(rep.AccessType), rep.LclNum, - m_currentBlock->bbNum); - - GenTree* readBack = Promotion::CreateReadBack(m_compiler, agg->LclNum, rep); - Statement* stmt = m_compiler->fgNewStmtFromTree(readBack); - DISPSTMT(stmt); - m_compiler->fgInsertStmtNearEnd(m_currentBlock, stmt); + if (materialize) + { + InsertReadBackAtEnd(m_currentBlock, agg->LclNum, rep); + } + else + { + BitVecOps::AddElemD(m_readBackTraits, state.PendingReadBacks, rep.ReadBackIndex); + } } else { @@ -2177,7 +2255,7 @@ void ReplaceVisitor::InsertPreStatementWriteBacks() // to its field local. // // We normally do this before the first use of the field we find, or before -// we transfer control to any successor. This method handles the case of +// control flow requires a materialized value. This method handles the case of // implicit control flow related to EH; when this basic block is in a // try-region (or filter block) and we find a tree that may throw it eagerly // inserts pending readbacks. @@ -2936,11 +3014,19 @@ PhaseStatus Promotion::Run() JITDUMP("Making replacements\n\n"); - // Make all replacements we decided on. - ReplaceVisitor replacer(this, aggregates, &liveness); - for (BasicBlock* bb : m_compiler->Blocks()) + // Make all replacements in reverse postorder so that forward predecessor + // states are available before processing a block. + if (m_compiler->m_dfsTree == nullptr) + { + m_compiler->m_dfsTree = m_compiler->fgComputeDfs(); + } + + FlowGraphDfsTree* dfsTree = m_compiler->m_dfsTree; + ReplaceVisitor replacer(this, aggregates, &liveness, dfsTree); + for (unsigned i = dfsTree->GetPostOrderCount(); i > 0; i--) { - Statement* firstStmt = replacer.StartBlock(bb); + BasicBlock* bb = dfsTree->GetPostOrder(i - 1); + Statement* firstStmt = replacer.StartBlock(bb); JITDUMP("\nReplacing in "); DBEXEC(m_compiler->verbose, bb->dspBlockHeader()); diff --git a/src/coreclr/jit/promotion.h b/src/coreclr/jit/promotion.h index db24874ad69992..532c39efce58a2 100644 --- a/src/coreclr/jit/promotion.h +++ b/src/coreclr/jit/promotion.h @@ -20,12 +20,13 @@ struct Replacement unsigned Offset; var_types AccessType; unsigned LclNum = BAD_VAR_NUM; + // Dense index into the inter-block pending-readback sets. + unsigned ReadBackIndex = BAD_VAR_NUM; // Is the replacement local (given by LclNum) fresher than the value in the struct local? bool NeedsWriteBack = true; // Is the value in the struct local fresher than the replacement local? - // Note that the invariant is that this is always false at the entrance to - // a basic block, i.e. all predecessors would have read the replacement - // back before transferring control if necessary. + // This may remain true across blocks when all incoming paths agree that + // the struct local contains the current value. bool NeedsReadBack = false; #ifdef DEBUG const char* Description = ""; @@ -248,6 +249,17 @@ class ReplaceVisitor : public GenTreeVisitor Statement* m_currentStmt = nullptr; BasicBlock* m_currentBlock = nullptr; + struct BlockState + { + BitVec PendingReadBacks; + bool Processed; + bool RequiresAlreadyReadBackOnEntry; + }; + + FlowGraphDfsTree* m_dfsTree; + BitVecTraits* m_readBackTraits; + BlockState* m_blockStates; + public: enum { @@ -256,13 +268,10 @@ class ReplaceVisitor : public GenTreeVisitor ComputeStack = true, }; - ReplaceVisitor(Promotion* prom, AggregateInfoMap& aggregates, PromotionLiveness* liveness) - : GenTreeVisitor(prom->m_compiler) - , m_promotion(prom) - , m_aggregates(aggregates) - , m_liveness(liveness) - { - } + ReplaceVisitor(Promotion* prom, + AggregateInfoMap& aggregates, + PromotionLiveness* liveness, + FlowGraphDfsTree* dfsTree); bool MadeChanges() { @@ -281,6 +290,8 @@ class ReplaceVisitor : public GenTreeVisitor fgWalkResult PostOrderVisit(GenTree** use, GenTree* user); private: + void InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNum, const Replacement& rep); + void SetNeedsWriteBack(Replacement& rep); void ClearNeedsWriteBack(Replacement& rep); void SetNeedsReadBack(Replacement& rep); From d255adee2bbb7cf8899a353baf8ff0875fbf2737 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 21 Sep 2026 11:04:45 +0200 Subject: [PATCH 3/7] JIT: Preserve physical promotion writeback state across blocks Index lazy-readback state by DFS postorder and use postorder bitsets for block flags. Track original-field currency across forward joins to avoid writing back replacements that have only been read, retaining conservative loop and handler entry state. Remove the low-weight parameter extraction costing heuristic now that readbacks can be deferred. Keep the existing pending-readback intersection policy at joins. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4c28b3c3-cf6c-4149-8eb1-964831492011 --- src/coreclr/jit/promotion.cpp | 107 +++++++++++++++++++--------------- src/coreclr/jit/promotion.h | 16 ++--- 2 files changed, 65 insertions(+), 58 deletions(-) diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index fd6bd821b698b7..5b2e9e83b47fb0 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -778,16 +778,7 @@ class LocalUses else if (lcl->lvIsParam) { // For parameters, the backend may be able to map it directly from a register. - // Small fields can pack many values into each parameter register, so eagerly - // extracting rarely used fields can add substantial work and register pressure. - // Wider fields naturally limit the number of extractions per register. - // Restrict the credit for small fields with few accesses to target these cases. - const weight_t MIN_RELATIVE_ACCESS_WEIGHT = 0.10; - bool allowBitwiseExtraction = - !varTypeIsSmall(access.AccessType) || - (access.CountWtd + inducedCountWtd) >= MIN_RELATIVE_ACCESS_WEIGHT * comp->fgFirstBB->getBBWeight(comp); - if (Promotion::MapsToParameterRegister(comp, lclNum, access.Offset, access.AccessType, - allowBitwiseExtraction)) + if (Promotion::MapsToParameterRegister(comp, lclNum, access.Offset, access.AccessType)) { // No promotion will result in a store to stack in the prolog. costWithout += COST_STRUCT_ACCESS_CYCLES * comp->fgFirstBB->getBBWeight(comp); @@ -1691,6 +1682,7 @@ ReplaceVisitor::ReplaceVisitor(Promotion* prom, , m_aggregates(aggregates) , m_liveness(liveness) , m_dfsTree(dfsTree) + , m_postOrderTraits(dfsTree->PostOrderTraits()) { unsigned index = 0; for (AggregateInfo* agg : m_aggregates) @@ -1701,19 +1693,25 @@ ReplaceVisitor::ReplaceVisitor(Promotion* prom, } } - m_readBackTraits = new (m_compiler, CMK_Promotion) BitVecTraits(index, m_compiler); - m_blockStates = new (m_compiler, CMK_Promotion) BlockState[m_compiler->fgBBNumMax + 1]{}; + m_readBackTraits = new (m_compiler, CMK_Promotion) BitVecTraits(index, m_compiler); + m_pendingReadBacks = new (m_compiler, CMK_Promotion) BitVec[dfsTree->GetPostOrderCount()]{}; + m_currentStructFields = new (m_compiler, CMK_Promotion) BitVec[dfsTree->GetPostOrderCount()]{}; + m_processedBlocks = BitVecOps::MakeEmpty(&m_postOrderTraits); + m_requiresAlreadyReadBackOnEntry = BitVecOps::MakeEmpty(&m_postOrderTraits); for (unsigned i = 0; i < dfsTree->GetPostOrderCount(); i++) { BasicBlock* block = dfsTree->GetPostOrder(i); - m_blockStates[block->bbNum].RequiresAlreadyReadBackOnEntry |= m_compiler->bbIsHandlerBeg(block); + if (m_compiler->bbIsHandlerBeg(block)) + { + BitVecOps::AddElemD(&m_postOrderTraits, m_requiresAlreadyReadBackOnEntry, block->bbPostorderNum); + } block->VisitRegularSuccs(m_compiler, [&](BasicBlock* succ) { if (succ->bbPostorderNum >= block->bbPostorderNum) { // This includes irreducible backedge targets: their incoming state // must be settled before visiting predecessors later in RPO. - m_blockStates[succ->bbNum].RequiresAlreadyReadBackOnEntry = true; + BitVecOps::AddElemD(&m_postOrderTraits, m_requiresAlreadyReadBackOnEntry, succ->bbPostorderNum); } return BasicBlockVisit::Continue; }); @@ -1722,7 +1720,7 @@ ReplaceVisitor::ReplaceVisitor(Promotion* prom, //------------------------------------------------------------------------ // StartBlock: -// Reconcile predecessor states and restore pending readbacks for this block. +// Reconcile predecessor states and restore readback/writeback status for this block. // // Parameters: // block - The block @@ -1759,15 +1757,18 @@ Statement* ReplaceVisitor::StartBlock(BasicBlock* block) continue; } - bool pending = false; + bool pending = false; + bool structCurrent = false; if (block == m_compiler->fgFirstBB) { - pending = dsc->lvIsParam || dsc->lvIsOSRLocal; + pending = dsc->lvIsParam || dsc->lvIsOSRLocal; + structCurrent = pending; } - else if (!m_blockStates[block->bbNum].RequiresAlreadyReadBackOnEntry) + else if (!BitVecOps::IsMember(&m_postOrderTraits, m_requiresAlreadyReadBackOnEntry, block->bbPostorderNum)) { - bool hasPred = false; - pending = true; + bool hasPred = false; + pending = true; + structCurrent = true; for (FlowEdge* edge : block->PredEdges()) { BasicBlock* pred = edge->getSourceBlock(); @@ -1776,17 +1777,25 @@ Statement* ReplaceVisitor::StartBlock(BasicBlock* block) continue; } - BlockState& state = m_blockStates[pred->bbNum]; - assert(state.Processed); + assert(BitVecOps::IsMember(&m_postOrderTraits, m_processedBlocks, pred->bbPostorderNum)); hasPred = true; - pending &= BitVecOps::IsMember(m_readBackTraits, state.PendingReadBacks, rep.ReadBackIndex); + pending &= BitVecOps::IsMember(m_readBackTraits, m_pendingReadBacks[pred->bbPostorderNum], + rep.ReadBackIndex); + structCurrent &= BitVecOps::IsMember(m_readBackTraits, m_currentStructFields[pred->bbPostorderNum], + rep.ReadBackIndex); } pending &= hasPred; + structCurrent &= hasPred; } - if (pending) + if (structCurrent) { ClearNeedsWriteBack(rep); + } + + if (pending) + { + assert(structCurrent); SetNeedsReadBack(rep); } else @@ -1802,12 +1811,13 @@ Statement* ReplaceVisitor::StartBlock(BasicBlock* block) continue; } - BlockState& state = m_blockStates[pred->bbNum]; - if (state.Processed && - BitVecOps::IsMember(m_readBackTraits, state.PendingReadBacks, rep.ReadBackIndex)) + if (BitVecOps::IsMember(&m_postOrderTraits, m_processedBlocks, pred->bbPostorderNum) && + BitVecOps::IsMember(m_readBackTraits, m_pendingReadBacks[pred->bbPostorderNum], + rep.ReadBackIndex)) { InsertReadBackAtEnd(pred, agg->LclNum, rep); - BitVecOps::RemoveElemD(m_readBackTraits, state.PendingReadBacks, rep.ReadBackIndex); + BitVecOps::RemoveElemD(m_readBackTraits, m_pendingReadBacks[pred->bbPostorderNum], + rep.ReadBackIndex); } } } @@ -1838,24 +1848,26 @@ void ReplaceVisitor::InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNu //------------------------------------------------------------------------ // EndBlock: -// Save pending readbacks for successors, materializing them at loop/EH boundaries. +// Save readback/writeback status for successors, materializing readbacks at loop/EH boundaries. // // Remarks: // Field descriptors are reset between visits; the saved state determines -// which replacements StartBlock can continue to leave in their struct homes. +// which replacements and original fields are current on entry to successors. // void ReplaceVisitor::EndBlock() { bool materialize = m_currentBlock->HasPotentialEHSuccs(m_compiler) || m_currentBlock->KindIs(BBJ_CALLFINALLY, BBJ_EHFINALLYRET, BBJ_EHFILTERRET, BBJ_EHCATCHRET); m_currentBlock->VisitRegularSuccs(m_compiler, [&](BasicBlock* succ) { - materialize |= m_blockStates[succ->bbNum].RequiresAlreadyReadBackOnEntry; + materialize |= BitVecOps::IsMember(&m_postOrderTraits, m_requiresAlreadyReadBackOnEntry, succ->bbPostorderNum); return BasicBlockVisit::Continue; }); - BlockState& state = m_blockStates[m_currentBlock->bbNum]; - state.PendingReadBacks = BitVecOps::MakeEmpty(m_readBackTraits); - state.Processed = true; + BitVec& pendingReadBacks = m_pendingReadBacks[m_currentBlock->bbPostorderNum]; + pendingReadBacks = BitVecOps::MakeEmpty(m_readBackTraits); + BitVec& currentStructFields = m_currentStructFields[m_currentBlock->bbPostorderNum]; + currentStructFields = BitVecOps::MakeEmpty(m_readBackTraits); + BitVecOps::AddElemD(&m_postOrderTraits, m_processedBlocks, m_currentBlock->bbPostorderNum); for (AggregateInfo* agg : m_aggregates) { @@ -1873,7 +1885,7 @@ void ReplaceVisitor::EndBlock() } else { - BitVecOps::AddElemD(m_readBackTraits, state.PendingReadBacks, rep.ReadBackIndex); + BitVecOps::AddElemD(m_readBackTraits, pendingReadBacks, rep.ReadBackIndex); } } else @@ -1902,6 +1914,13 @@ void ReplaceVisitor::EndBlock() ClearNeedsReadBack(rep); } + if (!rep.NeedsWriteBack) + { + // A readback leaves the original current too. Preserve that fact + // across blocks until a store to the replacement invalidates it. + BitVecOps::AddElemD(m_readBackTraits, currentStructFields, rep.ReadBackIndex); + } + SetNeedsWriteBack(rep); } } @@ -3158,17 +3177,15 @@ GenTree* Promotion::EffectiveUser(Compiler::GenTreeStack& ancestors) // expected to map to a register. // // Parameters: -// comp - Compiler instance -// lclNum - Local being accessed into -// offset - Offset being accessed at -// accessType - Type of access -// allowBitwiseExtraction - Whether to allow mappings requiring extraction or a register-class change +// comp - Compiler instance +// lclNum - Local being accessed into +// offset - Offset being accessed at +// accessType - Type of access // // Returns: // True if the access can be efficiently done via a parameter register. // -bool Promotion::MapsToParameterRegister( - Compiler* comp, unsigned lclNum, unsigned offset, var_types accessType, bool allowBitwiseExtraction) +bool Promotion::MapsToParameterRegister(Compiler* comp, unsigned lclNum, unsigned offset, var_types accessType) { assert(lclNum < comp->info.compArgsCount); @@ -3201,12 +3218,6 @@ bool Promotion::MapsToParameterRegister( continue; } - if (!allowBitwiseExtraction && ((offset != seg.Offset) || (genTypeSize(accessType) != seg.Size) || - (varTypeUsesIntReg(accessType) != genIsValidIntReg(seg.GetRegister())))) - { - continue; - } - return true; } diff --git a/src/coreclr/jit/promotion.h b/src/coreclr/jit/promotion.h index ee953a88e91119..a84c63c46bfc71 100644 --- a/src/coreclr/jit/promotion.h +++ b/src/coreclr/jit/promotion.h @@ -155,8 +155,7 @@ class Promotion static bool IsCandidateForPhysicalPromotion(LclVarDsc* dsc); static GenTree* EffectiveUser(Compiler::GenTreeStack& ancestors); - static bool MapsToParameterRegister( - Compiler* comp, unsigned lclNum, unsigned offs, var_types accessType, bool allowBitwiseExtraction = true); + static bool MapsToParameterRegister(Compiler* comp, unsigned lclNum, unsigned offs, var_types accessType); public: explicit Promotion(Compiler* compiler) : m_compiler(compiler) @@ -250,16 +249,13 @@ class ReplaceVisitor : public GenTreeVisitor Statement* m_currentStmt = nullptr; BasicBlock* m_currentBlock = nullptr; - struct BlockState - { - BitVec PendingReadBacks; - bool Processed; - bool RequiresAlreadyReadBackOnEntry; - }; - FlowGraphDfsTree* m_dfsTree; BitVecTraits* m_readBackTraits; - BlockState* m_blockStates; + BitVecTraits m_postOrderTraits; + BitVec* m_pendingReadBacks; + BitVec* m_currentStructFields; + BitVec m_processedBlocks; + BitVec m_requiresAlreadyReadBackOnEntry; public: enum From 7dae05a3526e37df92c2fcc5acbf94218aeb62cb Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 24 Sep 2026 15:18:32 +0200 Subject: [PATCH 4/7] Optimizing version --- src/coreclr/jit/promotion.cpp | 216 +++++++++++++++++- src/coreclr/jit/promotion.h | 30 ++- src/coreclr/jit/promotionliveness.cpp | 72 +++++- .../physicalpromotion/physicalpromotion.cs | 111 +++++++++ 4 files changed, 409 insertions(+), 20 deletions(-) diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index 5b2e9e83b47fb0..b3c0bfce3f34a9 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -1683,6 +1683,7 @@ ReplaceVisitor::ReplaceVisitor(Promotion* prom, , m_liveness(liveness) , m_dfsTree(dfsTree) , m_postOrderTraits(dfsTree->PostOrderTraits()) + , m_reconciliationReadBacks(prom->m_compiler->getAllocator(CMK_Promotion)) { unsigned index = 0; for (AggregateInfo* agg : m_aggregates) @@ -1718,6 +1719,209 @@ ReplaceVisitor::ReplaceVisitor(Promotion* prom, } } +//------------------------------------------------------------------------ +// OptimizeReadBacks: +// Common surviving initial readbacks introduced by mixed-join reconciliation. +// +// Remarks: +// Keep single readbacks and independent paths in their original positions. +// Only consider reads of the incoming value, and leave forwarded or embedded +// readbacks alone. Placing a shared readback late in the common dominator +// avoids extending its lifetime over unrelated work in that block. +// +void ReplaceVisitor::OptimizeReadBacks() +{ + if (m_reconciliationReadBacks.empty()) + { + return; + } + + struct ReadBackSite + { + BasicBlock* Block; + Statement* Stmt; + }; + + BasicBlock* entry = m_compiler->fgFirstBB; + FlowGraphDfsTree* placementDfs = m_dfsTree; + FlowGraphDominatorTree* domTree = m_compiler->m_domTree; + jitstd::vector sites(m_compiler->getAllocator(CMK_Promotion)); + ArrayStack worklist(m_compiler->getAllocator(CMK_Promotion)); + BitVec modified = BitVecOps::MakeEmpty(&m_postOrderTraits); + + for (AggregateInfo* agg : m_aggregates) + { + LclVarDsc* dsc = m_compiler->lvaGetDesc(agg->LclNum); + if (!dsc->lvIsParam && !dsc->lvIsOSRLocal) + { + continue; + } + + for (unsigned i = 0; i < agg->Replacements.size(); i++) + { + Replacement& rep = agg->Replacements[i]; + if (!rep.HasReconciledReadBack || !m_liveness->IsReplacementLiveIn(entry, agg->LclNum, i)) + { + continue; + } + + BitVecOps::ClearD(&m_postOrderTraits, modified); + auto markModified = [&](BasicBlock* block) { + if (m_dfsTree->Contains(block) && + BitVecOps::TryAddElemD(&m_postOrderTraits, modified, block->bbPostorderNum)) + { + worklist.Push(block); + } + return BasicBlockVisit::Continue; + }; + for (unsigned j = 0; j < m_dfsTree->GetPostOrderCount(); j++) + { + BasicBlock* block = m_dfsTree->GetPostOrder(j); + if (m_liveness->IsReplacementPossiblyDefined(block, agg->LclNum, i)) + { + markModified(block); + } + } + while (!worklist.Empty()) + { + worklist.Pop()->VisitAllSuccs(m_compiler, markModified); + } + + sites.clear(); + weight_t oldWeight = 0; + bool hasReconciliation = false; + for (unsigned j = 0; j < m_dfsTree->GetPostOrderCount(); j++) + { + BasicBlock* block = m_dfsTree->GetPostOrder(j); + if (BitVecOps::IsMember(&m_postOrderTraits, modified, block->bbPostorderNum)) + { + continue; + } + + for (Statement* stmt : block->Statements()) + { + GenTree* store = stmt->GetRootNode(); + if (!store->OperIs(GT_STORE_LCL_VAR) || (store->AsLclVarCommon()->GetLclNum() != rep.LclNum)) + { + continue; + } + + GenTree* value = store->Data(); + if (!value->OperIs(GT_LCL_FLD) || (value->AsLclFld()->GetLclNum() != agg->LclNum) || + (value->AsLclFld()->GetLclOffs() != rep.Offset)) + { + continue; + } + + sites.push_back({block, stmt}); + oldWeight += block->getBBWeight(m_compiler); + for (Statement* reconciliation : m_reconciliationReadBacks) + { + hasReconciliation |= stmt == reconciliation; + } + } + } + + if ((sites.size() < 2) || !hasReconciliation) + { + continue; + } + + if (domTree == nullptr) + { + if (entry->bbPostorderNum + 1 != m_dfsTree->GetPostOrderCount()) + { + // Before global morph the DFS may also retain the original + // OSR entry and a disconnected merged return. Dominators + // require a single root; its subtree is a postorder prefix. + placementDfs = new (m_compiler, CMK_Promotion) + FlowGraphDfsTree(m_compiler, m_dfsTree->GetPostOrder(), entry->bbPostorderNum + 1, + m_dfsTree->HasCycle(), m_dfsTree->IsProfileAware()); + } + domTree = FlowGraphDominatorTree::Build(placementDfs); + if (placementDfs == m_dfsTree) + { + m_compiler->m_domTree = domTree; + } + } + + BasicBlock* common = nullptr; + for (const ReadBackSite& site : sites) + { + if (!placementDfs->Contains(site.Block)) + { + common = nullptr; + break; + } + common = common == nullptr ? site.Block : domTree->Intersect(common, site.Block); + } + + if (common == nullptr) + { + continue; + } + + weight_t commonWeight = common->getBBWeight(m_compiler); + // If the sites merely partition execution, commoning saves no + // dynamic work and can hurt allocation by extending live ranges. + if ((commonWeight >= oldWeight) || + Compiler::fgProfileWeightsEqual(commonWeight, oldWeight, oldWeight * 1e-6)) + { + continue; + } + + assert(!BitVecOps::IsMember(&m_postOrderTraits, modified, common->bbPostorderNum)); + Statement* insertBefore = nullptr; + bool hasUse = m_liveness->IsReplacementUsed(common, agg->LclNum, i); + for (Statement* stmt : common->Statements()) + { + for (const ReadBackSite& site : sites) + { + if (site.Stmt == stmt) + { + insertBefore = stmt; + break; + } + } + if (hasUse) + { + for (GenTreeLclVarCommon* lcl : stmt->LocalsTreeList()) + { + if ((lcl->GetLclNum() == rep.LclNum) || (lcl->GetLclNum() == agg->LclNum)) + { + insertBefore = stmt; + break; + } + } + } + if (insertBefore != nullptr) + { + break; + } + } + + JITDUMP("Commoning %zu readbacks V%02u.[%03u..%03u) -> V%02u in " FMT_BB " (weight " FMT_WT + ", previous total " FMT_WT ")\n", + sites.size(), agg->LclNum, rep.Offset, rep.Offset + genTypeSize(rep.AccessType), rep.LclNum, + common->bbNum, common->getBBWeight(m_compiler), oldWeight); + Statement* readBack = + m_compiler->fgNewStmtFromTree(Promotion::CreateReadBack(m_compiler, agg->LclNum, rep)); + if (insertBefore != nullptr) + { + m_compiler->fgInsertStmtBefore(common, insertBefore, readBack); + } + else + { + m_compiler->fgInsertStmtNearEnd(common, readBack); + } + for (const ReadBackSite& site : sites) + { + m_compiler->fgRemoveStmt(site.Block, site.Stmt); + } + } + } +} + //------------------------------------------------------------------------ // StartBlock: // Reconcile predecessor states and restore readback/writeback status for this block. @@ -1815,7 +2019,7 @@ Statement* ReplaceVisitor::StartBlock(BasicBlock* block) BitVecOps::IsMember(m_readBackTraits, m_pendingReadBacks[pred->bbPostorderNum], rep.ReadBackIndex)) { - InsertReadBackAtEnd(pred, agg->LclNum, rep); + InsertReadBackAtEnd(pred, agg->LclNum, rep, true); BitVecOps::RemoveElemD(m_readBackTraits, m_pendingReadBacks[pred->bbPostorderNum], rep.ReadBackIndex); } @@ -1835,8 +2039,9 @@ Statement* ReplaceVisitor::StartBlock(BasicBlock* block) // block - Block in which the struct contains the current value. // structLclNum - Struct local. // rep - Replacement to initialize. +// reconcile - Whether this readback reconciles a mixed join. // -void ReplaceVisitor::InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNum, const Replacement& rep) +void ReplaceVisitor::InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNum, Replacement& rep, bool reconcile) { JITDUMP("Reading back V%02u.[%03u..%03u) -> V%02u near the end of " FMT_BB "\n", structLclNum, rep.Offset, rep.Offset + genTypeSize(rep.AccessType), rep.LclNum, block->bbNum); @@ -1844,6 +2049,11 @@ void ReplaceVisitor::InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNu GenTree* readBack = Promotion::CreateReadBack(m_compiler, structLclNum, rep); Statement* stmt = m_compiler->fgNewStmtFromTree(readBack); m_compiler->fgInsertStmtNearEnd(block, stmt); + if (reconcile) + { + rep.HasReconciledReadBack = true; + m_reconciliationReadBacks.push_back(stmt); + } } //------------------------------------------------------------------------ @@ -3089,6 +3299,8 @@ PhaseStatus Promotion::Run() replacer.EndBlock(); } + replacer.OptimizeReadBacks(); + // Add necessary explicit zeroing for some locals. Statement* prevStmt = nullptr; for (AggregateInfo* agg : aggregates) diff --git a/src/coreclr/jit/promotion.h b/src/coreclr/jit/promotion.h index a84c63c46bfc71..cdf7b9520753dc 100644 --- a/src/coreclr/jit/promotion.h +++ b/src/coreclr/jit/promotion.h @@ -27,7 +27,8 @@ struct Replacement // Is the value in the struct local fresher than the replacement local? // This may remain true across blocks when all incoming paths agree that // the struct local contains the current value. - bool NeedsReadBack = false; + bool NeedsReadBack = false; + bool HasReconciledReadBack = false; #ifdef DEBUG const char* Description = ""; #endif @@ -214,12 +215,19 @@ class PromotionLiveness } void Run(); + bool IsReplacementUsed(BasicBlock* bb, unsigned structLcl, unsigned replacement); + bool IsReplacementPossiblyDefined(BasicBlock* bb, unsigned structLcl, unsigned replacement); bool IsReplacementLiveIn(BasicBlock* bb, unsigned structLcl, unsigned replacement); bool IsReplacementLiveOut(BasicBlock* bb, unsigned structLcl, unsigned replacement); StructDeaths GetDeathsForStructLocal(GenTreeLclVarCommon* use); private: - void MarkUseDef(Statement* stmt, GenTreeLclVarCommon* lcl, BitVec& useSet, BitVec& defSet); + void MarkUseDef(Statement* stmt, + GenTreeLclVarCommon* lcl, + BitVec& useSet, + BitVec& defSet, + BitVec& mayDefSet, + bool conditional = false); unsigned GetSizeOfStructLocal(Statement* stmt, GenTreeLclVarCommon* lcl); void MarkIndex(unsigned index, bool isUse, bool isDef, BitVec& useSet, BitVec& defSet); void ComputeUseDefSets(); @@ -249,13 +257,14 @@ class ReplaceVisitor : public GenTreeVisitor Statement* m_currentStmt = nullptr; BasicBlock* m_currentBlock = nullptr; - FlowGraphDfsTree* m_dfsTree; - BitVecTraits* m_readBackTraits; - BitVecTraits m_postOrderTraits; - BitVec* m_pendingReadBacks; - BitVec* m_currentStructFields; - BitVec m_processedBlocks; - BitVec m_requiresAlreadyReadBackOnEntry; + FlowGraphDfsTree* m_dfsTree; + BitVecTraits* m_readBackTraits; + BitVecTraits m_postOrderTraits; + BitVec* m_pendingReadBacks; + BitVec* m_currentStructFields; + BitVec m_processedBlocks; + BitVec m_requiresAlreadyReadBackOnEntry; + jitstd::vector m_reconciliationReadBacks; public: enum @@ -283,11 +292,12 @@ class ReplaceVisitor : public GenTreeVisitor Statement* StartBlock(BasicBlock* block); void EndBlock(); void StartStatement(Statement* stmt); + void OptimizeReadBacks(); fgWalkResult PostOrderVisit(GenTree** use, GenTree* user); private: - void InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNum, const Replacement& rep); + void InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNum, Replacement& rep, bool reconcile = false); void SetNeedsWriteBack(Replacement& rep); void ClearNeedsWriteBack(Replacement& rep); diff --git a/src/coreclr/jit/promotionliveness.cpp b/src/coreclr/jit/promotionliveness.cpp index 8ba68f7711c5ca..7a626455e5b39f 100644 --- a/src/coreclr/jit/promotionliveness.cpp +++ b/src/coreclr/jit/promotionliveness.cpp @@ -48,6 +48,8 @@ struct BasicBlockLiveness // Note that this differs from our normal liveness: partial definitions are // NOT marked but they are also not considered uses. BitVec VarDef; + // Any definitions, including partial and conditionally executed definitions. + BitVec VarMayDef; // Variables live-in to this basic block. BitVec LiveIn; // Variables live-out of this basic block. @@ -108,6 +110,7 @@ void PromotionLiveness::ComputeUseDefSets() BasicBlockLiveness& bb = m_bbInfo[block->bbNum]; BitVecOps::AssignNoCopy(m_bvTraits, bb.VarUse, BitVecOps::MakeEmpty(m_bvTraits)); BitVecOps::AssignNoCopy(m_bvTraits, bb.VarDef, BitVecOps::MakeEmpty(m_bvTraits)); + BitVecOps::AssignNoCopy(m_bvTraits, bb.VarMayDef, BitVecOps::MakeEmpty(m_bvTraits)); BitVecOps::AssignNoCopy(m_bvTraits, bb.LiveIn, BitVecOps::MakeEmpty(m_bvTraits)); BitVecOps::AssignNoCopy(m_bvTraits, bb.LiveOut, BitVecOps::MakeEmpty(m_bvTraits)); @@ -121,18 +124,14 @@ void PromotionLiveness::ComputeUseDefSets() { for (GenTreeLclVarCommon* lcl : stmt->LocalsTreeList()) { - MarkUseDef(stmt, lcl, bb.VarUse, bb.VarDef); + MarkUseDef(stmt, lcl, bb.VarUse, bb.VarDef, bb.VarMayDef); } } else { for (GenTreeLclVarCommon* lcl : stmt->LocalsTreeList()) { - // Skip liveness updates/marking for defs; they may be conditionally executed. - if ((lcl->gtFlags & GTF_VAR_DEF) == 0) - { - MarkUseDef(stmt, lcl, bb.VarUse, bb.VarDef); - } + MarkUseDef(stmt, lcl, bb.VarUse, bb.VarDef, bb.VarMayDef, true); } } } @@ -143,7 +142,7 @@ void PromotionLiveness::ComputeUseDefSets() { for (GenTreeLclVarCommon* lcl : stmt->LocalsTreeList()) { - MarkUseDef(stmt, lcl, bb.VarUse, bb.VarDef); + MarkUseDef(stmt, lcl, bb.VarUse, bb.VarDef, bb.VarMayDef); } } } @@ -171,8 +170,11 @@ void PromotionLiveness::ComputeUseDefSets() // lcl - The local node // useSet - The use set to mark in. // defSet - The def set to mark in. +// mayDefSet - The set of all potentially modified fields. +// conditional - Whether definitions may be conditionally executed. // -void PromotionLiveness::MarkUseDef(Statement* stmt, GenTreeLclVarCommon* lcl, BitVec& useSet, BitVec& defSet) +void PromotionLiveness::MarkUseDef( + Statement* stmt, GenTreeLclVarCommon* lcl, BitVec& useSet, BitVec& defSet, BitVec& mayDefSet, bool conditional) { AggregateInfo* agg = m_aggregates.Lookup(lcl->GetLclNum()); if (agg == nullptr) @@ -187,6 +189,24 @@ void PromotionLiveness::MarkUseDef(Statement* stmt, GenTreeLclVarCommon* lcl, Bi unsigned baseIndex = m_structLclToTrackedIndex[lcl->GetLclNum()]; var_types accessType = lcl->TypeGet(); + if (isDef) + { + unsigned size = (accessType == TYP_STRUCT) || lcl->OperIs(GT_LCL_ADDR) ? GetSizeOfStructLocal(stmt, lcl) + : genTypeSize(accessType); + for (unsigned i = 0; i < reps.size(); i++) + { + if (reps[i].Overlaps(lcl->GetLclOffs(), size)) + { + BitVecOps::AddElemD(m_bvTraits, mayDefSet, baseIndex + 1 + i); + } + } + + if (conditional) + { + return; + } + } + if ((accessType == TYP_STRUCT) || lcl->OperIs(GT_LCL_ADDR)) { if (lcl->OperIsScalarLocal()) @@ -646,6 +666,42 @@ void PromotionLiveness::FillInLiveness(BitVec& life, BitVec volatileVars, Statem } } +//------------------------------------------------------------------------ +// IsReplacementUsed: +// Check if a replacement field is used before being defined in a block. +// +// Parameters: +// bb - The block +// structLcl - The struct (base) local +// replacementIndex - Index of the replacement +// +// Returns: +// True if the field is in the upward-exposed use set. +// +bool PromotionLiveness::IsReplacementUsed(BasicBlock* bb, unsigned structLcl, unsigned replacementIndex) +{ + unsigned index = m_structLclToTrackedIndex[structLcl] + 1 + replacementIndex; + return BitVecOps::IsMember(m_bvTraits, m_bbInfo[bb->bbNum].VarUse, index); +} + +//------------------------------------------------------------------------ +// IsReplacementPossiblyDefined: +// Check if any part of a replacement field may be defined in a block. +// +// Parameters: +// bb - The block +// structLcl - The struct (base) local +// replacementIndex - Index of the replacement +// +// Returns: +// True if the field is in the may-definition set. +// +bool PromotionLiveness::IsReplacementPossiblyDefined(BasicBlock* bb, unsigned structLcl, unsigned replacementIndex) +{ + unsigned index = m_structLclToTrackedIndex[structLcl] + 1 + replacementIndex; + return BitVecOps::IsMember(m_bvTraits, m_bbInfo[bb->bbNum].VarMayDef, index); +} + //------------------------------------------------------------------------ // IsReplacementLiveIn: // Check if a replacement field is live at the start of a basic block. diff --git a/src/tests/JIT/Directed/physicalpromotion/physicalpromotion.cs b/src/tests/JIT/Directed/physicalpromotion/physicalpromotion.cs index 57c6049ca24979..e01842d91a34b5 100644 --- a/src/tests/JIT/Directed/physicalpromotion/physicalpromotion.cs +++ b/src/tests/JIT/Directed/physicalpromotion/physicalpromotion.cs @@ -104,6 +104,117 @@ public static void Overlappy() Consume(lcl3); } + [Theory] + [InlineData(0, 0U)] + [InlineData(1, 60U)] + [InlineData(2, 120U)] + [InlineData(3, 54U)] + [InlineData(4, 49U)] + [InlineData(5, 70U)] + [InlineData(6, 0x56781245U)] + [InlineData(7, 46U)] + public static void ReadbacksAcrossBranches(int path, uint expected) + { + S value = new S { A = 17, B = 29 }; + Assert.Equal(expected, path is 6 ? ReadbackAfterPartialWrite(value) : ReadbacksAcrossBranchesCore(value, path)); + Assert.Equal(path is 0 ? 0U : path is 1 ? 1U : 46U, ReadbackOnlyOnSelectedPath(value, path)); + Assert.Equal(path switch { 0 => 29U, 1 => 63U, 2 => 80U, _ => 46U }, ReadbacksAtLiveJoin(value, path)); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static uint ReadbacksAtLiveJoin(S value, int path) + { + if (path == 0) + { + return value.B; + } + + uint result = 0; + if (path == 1) + { + result = value.A; + } + else if (path == 2) + { + result = value.A * 2; + } + + return result + value.A + value.B; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static uint ReadbackAfterPartialWrite(S value) + { + value.C = 0x12345678; + return value.A + value.B; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static uint ReadbackOnlyOnSelectedPath(S value, int path) + { + if (path == 0) + { + return 0; + } + + if (path == 1) + { + return 1; + } + + return value.A + value.B; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static uint ReadbacksAcrossBranchesCore(S value, int path) + { + if (path == 0) + { + return 0; + } + + Consume(value); + switch (path) + { + case 1: + value.A = 31; + break; + case 2: + value = GetReadbackValue(); + break; + case 3: + value.B = 37; + break; + case 4: + for (int i = 0; i < 3; i++) + { + value.A += (uint)i; + Consume(value); + } + break; + case 5: + try + { + value.A = 41; + ThrowForReadback(); + } + catch (InvalidOperationException) + { + Consume(value); + } + break; + } + + Consume(value); + return value.A + value.B; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static S GetReadbackValue() => new S { A = 73, B = 47 }; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void ThrowForReadback() => throw new InvalidOperationException(); + [MethodImpl(MethodImplOptions.NoInlining)] private static void Consume(T val) { From ee097b44dda9652cac3ab11dc7041e09d74323bc Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 25 Sep 2026 12:40:32 +0200 Subject: [PATCH 5/7] JIT: Plan common readbacks before physical promotion replacement Estimate lazy readback and reconciliation sites from liveness use/def sets. Let replacement materialize planned readbacks using its exact state, removing post-replacement commoning and may-definition tracking. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 628eb6b2-f468-4a67-b2c9-f8de24969633 --- src/coreclr/jit/promotion.cpp | 252 ++++++++---------- src/coreclr/jit/promotion.h | 39 ++- src/coreclr/jit/promotionliveness.cpp | 46 +--- .../physicalpromotion/physicalpromotion.cs | 11 +- 4 files changed, 156 insertions(+), 192 deletions(-) diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index b3c0bfce3f34a9..d2cebaa76a7e35 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -1683,7 +1683,6 @@ ReplaceVisitor::ReplaceVisitor(Promotion* prom, , m_liveness(liveness) , m_dfsTree(dfsTree) , m_postOrderTraits(dfsTree->PostOrderTraits()) - , m_reconciliationReadBacks(prom->m_compiler->getAllocator(CMK_Promotion)) { unsigned index = 0; for (AggregateInfo* agg : m_aggregates) @@ -1717,37 +1716,38 @@ ReplaceVisitor::ReplaceVisitor(Promotion* prom, return BasicBlockVisit::Continue; }); } + + PlanReadBacks(); } //------------------------------------------------------------------------ -// OptimizeReadBacks: -// Common surviving initial readbacks introduced by mixed-join reconciliation. +// PlanReadBacks: +// Estimate lazy readbacks from liveness use/def sets and choose shared +// materialization points for fields requiring mixed-join reconciliation. // // Remarks: -// Keep single readbacks and independent paths in their original positions. -// Only consider reads of the incoming value, and leave forwarded or embedded -// readbacks alone. Placing a shared readback late in the common dominator -// avoids extending its lifetime over unrelated work in that block. +// This is a profitability model, not a correctness analysis. Uses are assumed +// to materialize pending fields, and definitions end the incoming value. +// Replacement retains its exact state transitions and inserts any readbacks +// still required, including after partial writes and at EH boundaries. // -void ReplaceVisitor::OptimizeReadBacks() +void ReplaceVisitor::PlanReadBacks() { - if (m_reconciliationReadBacks.empty()) - { - return; - } + BasicBlock* entry = m_compiler->fgFirstBB; + FlowGraphDfsTree* placementDfs = m_dfsTree; + FlowGraphDominatorTree* domTree = m_compiler->m_domTree; + BitVec pendingOut = BitVecOps::MakeEmpty(&m_postOrderTraits); + BitVec sites = BitVecOps::MakeEmpty(&m_postOrderTraits); + BitVec barriers = BitVecOps::MakeEmpty(&m_postOrderTraits); - struct ReadBackSite + for (unsigned i = 0; i < m_dfsTree->GetPostOrderCount(); i++) { - BasicBlock* Block; - Statement* Stmt; - }; - - BasicBlock* entry = m_compiler->fgFirstBB; - FlowGraphDfsTree* placementDfs = m_dfsTree; - FlowGraphDominatorTree* domTree = m_compiler->m_domTree; - jitstd::vector sites(m_compiler->getAllocator(CMK_Promotion)); - ArrayStack worklist(m_compiler->getAllocator(CMK_Promotion)); - BitVec modified = BitVecOps::MakeEmpty(&m_postOrderTraits); + BasicBlock* block = m_dfsTree->GetPostOrder(i); + if (MustMaterializeReadBacks(block)) + { + BitVecOps::AddElemD(&m_postOrderTraits, barriers, i); + } + } for (AggregateInfo* agg : m_aggregates) { @@ -1760,69 +1760,78 @@ void ReplaceVisitor::OptimizeReadBacks() for (unsigned i = 0; i < agg->Replacements.size(); i++) { Replacement& rep = agg->Replacements[i]; - if (!rep.HasReconciledReadBack || !m_liveness->IsReplacementLiveIn(entry, agg->LclNum, i)) + if (!m_liveness->IsReplacementLiveIn(entry, agg->LclNum, i)) { continue; } - BitVecOps::ClearD(&m_postOrderTraits, modified); - auto markModified = [&](BasicBlock* block) { - if (m_dfsTree->Contains(block) && - BitVecOps::TryAddElemD(&m_postOrderTraits, modified, block->bbPostorderNum)) - { - worklist.Push(block); - } - return BasicBlockVisit::Continue; - }; - for (unsigned j = 0; j < m_dfsTree->GetPostOrderCount(); j++) - { - BasicBlock* block = m_dfsTree->GetPostOrder(j); - if (m_liveness->IsReplacementPossiblyDefined(block, agg->LclNum, i)) - { - markModified(block); - } - } - while (!worklist.Empty()) - { - worklist.Pop()->VisitAllSuccs(m_compiler, markModified); - } - - sites.clear(); - weight_t oldWeight = 0; - bool hasReconciliation = false; - for (unsigned j = 0; j < m_dfsTree->GetPostOrderCount(); j++) + BitVecOps::ClearD(&m_postOrderTraits, pendingOut); + BitVecOps::ClearD(&m_postOrderTraits, sites); + bool hasReconciliation = false; + for (unsigned j = m_dfsTree->GetPostOrderCount(); j > 0; j--) { - BasicBlock* block = m_dfsTree->GetPostOrder(j); - if (BitVecOps::IsMember(&m_postOrderTraits, modified, block->bbPostorderNum)) + BasicBlock* block = m_dfsTree->GetPostOrder(j - 1); + if (!m_liveness->IsReplacementLiveIn(block, agg->LclNum, i)) { continue; } - for (Statement* stmt : block->Statements()) + bool pending = block == entry; + if ((block != entry) && + !BitVecOps::IsMember(&m_postOrderTraits, m_requiresAlreadyReadBackOnEntry, block->bbPostorderNum)) { - GenTree* store = stmt->GetRootNode(); - if (!store->OperIs(GT_STORE_LCL_VAR) || (store->AsLclVarCommon()->GetLclNum() != rep.LclNum)) + bool anyPending = false; + bool allPending = true; + for (FlowEdge* edge : block->PredEdges()) { - continue; + BasicBlock* pred = edge->getSourceBlock(); + if (!m_dfsTree->Contains(pred)) + { + continue; + } + bool predPending = BitVecOps::IsMember(&m_postOrderTraits, pendingOut, pred->bbPostorderNum); + anyPending |= predPending; + allPending &= predPending; } - - GenTree* value = store->Data(); - if (!value->OperIs(GT_LCL_FLD) || (value->AsLclFld()->GetLclNum() != agg->LclNum) || - (value->AsLclFld()->GetLclOffs() != rep.Offset)) + pending = anyPending && allPending; + if (anyPending && !allPending) { - continue; + hasReconciliation = true; + for (FlowEdge* edge : block->PredEdges()) + { + BasicBlock* pred = edge->getSourceBlock(); + if (m_dfsTree->Contains(pred) && + BitVecOps::IsMember(&m_postOrderTraits, pendingOut, pred->bbPostorderNum)) + { + BitVecOps::AddElemD(&m_postOrderTraits, sites, pred->bbPostorderNum); + BitVecOps::RemoveElemD(&m_postOrderTraits, pendingOut, pred->bbPostorderNum); + } + } } - - sites.push_back({block, stmt}); - oldWeight += block->getBBWeight(m_compiler); - for (Statement* reconciliation : m_reconciliationReadBacks) + } + if (pending && m_liveness->IsReplacementUsed(block, agg->LclNum, i)) + { + BitVecOps::AddElemD(&m_postOrderTraits, sites, block->bbPostorderNum); + pending = false; + } + if (m_liveness->IsReplacementDefined(block, agg->LclNum, i)) + { + pending = false; + } + if (pending && m_liveness->IsReplacementLiveOut(block, agg->LclNum, i)) + { + if (BitVecOps::IsMember(&m_postOrderTraits, barriers, block->bbPostorderNum)) + { + BitVecOps::AddElemD(&m_postOrderTraits, sites, block->bbPostorderNum); + } + else { - hasReconciliation |= stmt == reconciliation; + BitVecOps::AddElemD(&m_postOrderTraits, pendingOut, block->bbPostorderNum); } } } - if ((sites.size() < 2) || !hasReconciliation) + if (!hasReconciliation || (BitVecOps::Count(&m_postOrderTraits, sites) < 2)) { continue; } @@ -1845,15 +1854,20 @@ void ReplaceVisitor::OptimizeReadBacks() } } - BasicBlock* common = nullptr; - for (const ReadBackSite& site : sites) + BasicBlock* common = nullptr; + weight_t oldWeight = 0; + BitVecOps::Iter iter(&m_postOrderTraits, sites); + unsigned j; + while (iter.NextElem(&j)) { - if (!placementDfs->Contains(site.Block)) + BasicBlock* block = m_dfsTree->GetPostOrder(j); + if (!placementDfs->Contains(block)) { common = nullptr; break; } - common = common == nullptr ? site.Block : domTree->Intersect(common, site.Block); + common = common == nullptr ? block : domTree->Intersect(common, block); + oldWeight += block->getBBWeight(m_compiler); } if (common == nullptr) @@ -1870,54 +1884,12 @@ void ReplaceVisitor::OptimizeReadBacks() continue; } - assert(!BitVecOps::IsMember(&m_postOrderTraits, modified, common->bbPostorderNum)); - Statement* insertBefore = nullptr; - bool hasUse = m_liveness->IsReplacementUsed(common, agg->LclNum, i); - for (Statement* stmt : common->Statements()) - { - for (const ReadBackSite& site : sites) - { - if (site.Stmt == stmt) - { - insertBefore = stmt; - break; - } - } - if (hasUse) - { - for (GenTreeLclVarCommon* lcl : stmt->LocalsTreeList()) - { - if ((lcl->GetLclNum() == rep.LclNum) || (lcl->GetLclNum() == agg->LclNum)) - { - insertBefore = stmt; - break; - } - } - } - if (insertBefore != nullptr) - { - break; - } - } - - JITDUMP("Commoning %zu readbacks V%02u.[%03u..%03u) -> V%02u in " FMT_BB " (weight " FMT_WT - ", previous total " FMT_WT ")\n", - sites.size(), agg->LclNum, rep.Offset, rep.Offset + genTypeSize(rep.AccessType), rep.LclNum, - common->bbNum, common->getBBWeight(m_compiler), oldWeight); - Statement* readBack = - m_compiler->fgNewStmtFromTree(Promotion::CreateReadBack(m_compiler, agg->LclNum, rep)); - if (insertBefore != nullptr) - { - m_compiler->fgInsertStmtBefore(common, insertBefore, readBack); - } - else - { - m_compiler->fgInsertStmtNearEnd(common, readBack); - } - for (const ReadBackSite& site : sites) - { - m_compiler->fgRemoveStmt(site.Block, site.Stmt); - } + rep.ReadBackPlacement = common; + JITDUMP("Planning common readback for %u estimated sites V%02u.[%03u..%03u) -> V%02u in " FMT_BB + " (weight " FMT_WT ", previous total " FMT_WT ")\n", + BitVecOps::Count(&m_postOrderTraits, sites), agg->LclNum, rep.Offset, + rep.Offset + genTypeSize(rep.AccessType), rep.LclNum, common->bbNum, + common->getBBWeight(m_compiler), oldWeight); } } } @@ -2019,7 +1991,7 @@ Statement* ReplaceVisitor::StartBlock(BasicBlock* block) BitVecOps::IsMember(m_readBackTraits, m_pendingReadBacks[pred->bbPostorderNum], rep.ReadBackIndex)) { - InsertReadBackAtEnd(pred, agg->LclNum, rep, true); + InsertReadBackAtEnd(pred, agg->LclNum, rep); BitVecOps::RemoveElemD(m_readBackTraits, m_pendingReadBacks[pred->bbPostorderNum], rep.ReadBackIndex); } @@ -2039,9 +2011,8 @@ Statement* ReplaceVisitor::StartBlock(BasicBlock* block) // block - Block in which the struct contains the current value. // structLclNum - Struct local. // rep - Replacement to initialize. -// reconcile - Whether this readback reconciles a mixed join. // -void ReplaceVisitor::InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNum, Replacement& rep, bool reconcile) +void ReplaceVisitor::InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNum, Replacement& rep) { JITDUMP("Reading back V%02u.[%03u..%03u) -> V%02u near the end of " FMT_BB "\n", structLclNum, rep.Offset, rep.Offset + genTypeSize(rep.AccessType), rep.LclNum, block->bbNum); @@ -2049,11 +2020,27 @@ void ReplaceVisitor::InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNu GenTree* readBack = Promotion::CreateReadBack(m_compiler, structLclNum, rep); Statement* stmt = m_compiler->fgNewStmtFromTree(readBack); m_compiler->fgInsertStmtNearEnd(block, stmt); - if (reconcile) - { - rep.HasReconciledReadBack = true; - m_reconciliationReadBacks.push_back(stmt); - } +} + +//------------------------------------------------------------------------ +// MustMaterializeReadBacks: +// Check whether pending readbacks must be materialized before leaving a block. +// +// Parameters: +// block - The block to check. +// +// Returns: +// True at loop/EH boundaries requiring current replacement locals. +// +bool ReplaceVisitor::MustMaterializeReadBacks(BasicBlock* block) +{ + bool materialize = block->HasPotentialEHSuccs(m_compiler) || + block->KindIs(BBJ_CALLFINALLY, BBJ_EHFINALLYRET, BBJ_EHFILTERRET, BBJ_EHCATCHRET); + block->VisitRegularSuccs(m_compiler, [&](BasicBlock* succ) { + materialize |= BitVecOps::IsMember(&m_postOrderTraits, m_requiresAlreadyReadBackOnEntry, succ->bbPostorderNum); + return BasicBlockVisit::Continue; + }); + return materialize; } //------------------------------------------------------------------------ @@ -2066,12 +2053,7 @@ void ReplaceVisitor::InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNu // void ReplaceVisitor::EndBlock() { - bool materialize = m_currentBlock->HasPotentialEHSuccs(m_compiler) || - m_currentBlock->KindIs(BBJ_CALLFINALLY, BBJ_EHFINALLYRET, BBJ_EHFILTERRET, BBJ_EHCATCHRET); - m_currentBlock->VisitRegularSuccs(m_compiler, [&](BasicBlock* succ) { - materialize |= BitVecOps::IsMember(&m_postOrderTraits, m_requiresAlreadyReadBackOnEntry, succ->bbPostorderNum); - return BasicBlockVisit::Continue; - }); + bool materialize = MustMaterializeReadBacks(m_currentBlock); BitVec& pendingReadBacks = m_pendingReadBacks[m_currentBlock->bbPostorderNum]; pendingReadBacks = BitVecOps::MakeEmpty(m_readBackTraits); @@ -2089,7 +2071,7 @@ void ReplaceVisitor::EndBlock() { if (m_liveness->IsReplacementLiveOut(m_currentBlock, agg->LclNum, (unsigned)i)) { - if (materialize) + if (materialize || (rep.ReadBackPlacement == m_currentBlock)) { InsertReadBackAtEnd(m_currentBlock, agg->LclNum, rep); } @@ -3299,8 +3281,6 @@ PhaseStatus Promotion::Run() replacer.EndBlock(); } - replacer.OptimizeReadBacks(); - // Add necessary explicit zeroing for some locals. Statement* prevStmt = nullptr; for (AggregateInfo* agg : aggregates) diff --git a/src/coreclr/jit/promotion.h b/src/coreclr/jit/promotion.h index cdf7b9520753dc..3ce8123a220aef 100644 --- a/src/coreclr/jit/promotion.h +++ b/src/coreclr/jit/promotion.h @@ -17,9 +17,10 @@ const int PHYSICAL_PROMOTION_MAX_PROMOTIONS_PER_STRUCT = 64; // Represents a single replacement of a (field) access into a struct local. struct Replacement { - unsigned Offset; - var_types AccessType; - unsigned LclNum = BAD_VAR_NUM; + BasicBlock* ReadBackPlacement = nullptr; + unsigned Offset; + var_types AccessType; + unsigned LclNum = BAD_VAR_NUM; // Dense index into the inter-block pending-readback sets. unsigned ReadBackIndex = BAD_VAR_NUM; // Is the replacement local (given by LclNum) fresher than the value in the struct local? @@ -27,8 +28,7 @@ struct Replacement // Is the value in the struct local fresher than the replacement local? // This may remain true across blocks when all incoming paths agree that // the struct local contains the current value. - bool NeedsReadBack = false; - bool HasReconciledReadBack = false; + bool NeedsReadBack = false; #ifdef DEBUG const char* Description = ""; #endif @@ -216,18 +216,13 @@ class PromotionLiveness void Run(); bool IsReplacementUsed(BasicBlock* bb, unsigned structLcl, unsigned replacement); - bool IsReplacementPossiblyDefined(BasicBlock* bb, unsigned structLcl, unsigned replacement); + bool IsReplacementDefined(BasicBlock* bb, unsigned structLcl, unsigned replacement); bool IsReplacementLiveIn(BasicBlock* bb, unsigned structLcl, unsigned replacement); bool IsReplacementLiveOut(BasicBlock* bb, unsigned structLcl, unsigned replacement); StructDeaths GetDeathsForStructLocal(GenTreeLclVarCommon* use); private: - void MarkUseDef(Statement* stmt, - GenTreeLclVarCommon* lcl, - BitVec& useSet, - BitVec& defSet, - BitVec& mayDefSet, - bool conditional = false); + void MarkUseDef(Statement* stmt, GenTreeLclVarCommon* lcl, BitVec& useSet, BitVec& defSet); unsigned GetSizeOfStructLocal(Statement* stmt, GenTreeLclVarCommon* lcl); void MarkIndex(unsigned index, bool isUse, bool isDef, BitVec& useSet, BitVec& defSet); void ComputeUseDefSets(); @@ -257,14 +252,13 @@ class ReplaceVisitor : public GenTreeVisitor Statement* m_currentStmt = nullptr; BasicBlock* m_currentBlock = nullptr; - FlowGraphDfsTree* m_dfsTree; - BitVecTraits* m_readBackTraits; - BitVecTraits m_postOrderTraits; - BitVec* m_pendingReadBacks; - BitVec* m_currentStructFields; - BitVec m_processedBlocks; - BitVec m_requiresAlreadyReadBackOnEntry; - jitstd::vector m_reconciliationReadBacks; + FlowGraphDfsTree* m_dfsTree; + BitVecTraits* m_readBackTraits; + BitVecTraits m_postOrderTraits; + BitVec* m_pendingReadBacks; + BitVec* m_currentStructFields; + BitVec m_processedBlocks; + BitVec m_requiresAlreadyReadBackOnEntry; public: enum @@ -292,12 +286,13 @@ class ReplaceVisitor : public GenTreeVisitor Statement* StartBlock(BasicBlock* block); void EndBlock(); void StartStatement(Statement* stmt); - void OptimizeReadBacks(); fgWalkResult PostOrderVisit(GenTree** use, GenTree* user); private: - void InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNum, Replacement& rep, bool reconcile = false); + void PlanReadBacks(); + bool MustMaterializeReadBacks(BasicBlock* block); + void InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNum, Replacement& rep); void SetNeedsWriteBack(Replacement& rep); void ClearNeedsWriteBack(Replacement& rep); diff --git a/src/coreclr/jit/promotionliveness.cpp b/src/coreclr/jit/promotionliveness.cpp index 5f53923e6d467e..c3632d633a9260 100644 --- a/src/coreclr/jit/promotionliveness.cpp +++ b/src/coreclr/jit/promotionliveness.cpp @@ -48,8 +48,6 @@ struct BasicBlockLiveness // Note that this differs from our normal liveness: partial definitions are // NOT marked but they are also not considered uses. BitVec VarDef; - // Any definitions, including partial and conditionally executed definitions. - BitVec VarMayDef; // Variables live-in to this basic block. BitVec LiveIn; // Variables live-out of this basic block. @@ -110,7 +108,6 @@ void PromotionLiveness::ComputeUseDefSets() BasicBlockLiveness& bb = m_bbInfo[block->bbNum]; BitVecOps::AssignNoCopy(m_bvTraits, bb.VarUse, BitVecOps::MakeEmpty(m_bvTraits)); BitVecOps::AssignNoCopy(m_bvTraits, bb.VarDef, BitVecOps::MakeEmpty(m_bvTraits)); - BitVecOps::AssignNoCopy(m_bvTraits, bb.VarMayDef, BitVecOps::MakeEmpty(m_bvTraits)); BitVecOps::AssignNoCopy(m_bvTraits, bb.LiveIn, BitVecOps::MakeEmpty(m_bvTraits)); BitVecOps::AssignNoCopy(m_bvTraits, bb.LiveOut, BitVecOps::MakeEmpty(m_bvTraits)); @@ -124,14 +121,18 @@ void PromotionLiveness::ComputeUseDefSets() { for (GenTreeLclVarCommon* lcl : stmt->LocalsTreeList()) { - MarkUseDef(stmt, lcl, bb.VarUse, bb.VarDef, bb.VarMayDef); + MarkUseDef(stmt, lcl, bb.VarUse, bb.VarDef); } } else { for (GenTreeLclVarCommon* lcl : stmt->LocalsTreeList()) { - MarkUseDef(stmt, lcl, bb.VarUse, bb.VarDef, bb.VarMayDef, true); + // Skip liveness updates/marking for defs; they may be conditionally executed. + if ((lcl->gtFlags & GTF_VAR_DEF) == 0) + { + MarkUseDef(stmt, lcl, bb.VarUse, bb.VarDef); + } } } } @@ -142,7 +143,7 @@ void PromotionLiveness::ComputeUseDefSets() { for (GenTreeLclVarCommon* lcl : stmt->LocalsTreeList()) { - MarkUseDef(stmt, lcl, bb.VarUse, bb.VarDef, bb.VarMayDef); + MarkUseDef(stmt, lcl, bb.VarUse, bb.VarDef); } } } @@ -170,11 +171,8 @@ void PromotionLiveness::ComputeUseDefSets() // lcl - The local node // useSet - The use set to mark in. // defSet - The def set to mark in. -// mayDefSet - The set of all potentially modified fields. -// conditional - Whether definitions may be conditionally executed. // -void PromotionLiveness::MarkUseDef( - Statement* stmt, GenTreeLclVarCommon* lcl, BitVec& useSet, BitVec& defSet, BitVec& mayDefSet, bool conditional) +void PromotionLiveness::MarkUseDef(Statement* stmt, GenTreeLclVarCommon* lcl, BitVec& useSet, BitVec& defSet) { AggregateInfo* agg = m_aggregates.Lookup(lcl->GetLclNum()); if (agg == nullptr) @@ -189,24 +187,6 @@ void PromotionLiveness::MarkUseDef( unsigned baseIndex = m_structLclToTrackedIndex[lcl->GetLclNum()]; var_types accessType = lcl->TypeGet(); - if (isDef) - { - unsigned size = (accessType == TYP_STRUCT) || lcl->OperIs(GT_LCL_ADDR) ? GetSizeOfStructLocal(stmt, lcl) - : genTypeSize(accessType); - for (unsigned i = 0; i < reps.size(); i++) - { - if (reps[i].Overlaps(lcl->GetLclOffs(), size)) - { - BitVecOps::AddElemD(m_bvTraits, mayDefSet, baseIndex + 1 + i); - } - } - - if (conditional) - { - return; - } - } - if ((accessType == TYP_STRUCT) || lcl->OperIs(GT_LCL_ADDR)) { if (lcl->OperIsScalarLocal()) @@ -685,8 +665,8 @@ bool PromotionLiveness::IsReplacementUsed(BasicBlock* bb, unsigned structLcl, un } //------------------------------------------------------------------------ -// IsReplacementPossiblyDefined: -// Check if any part of a replacement field may be defined in a block. +// IsReplacementDefined: +// Check if a replacement field is fully defined in a block. // // Parameters: // bb - The block @@ -694,12 +674,12 @@ bool PromotionLiveness::IsReplacementUsed(BasicBlock* bb, unsigned structLcl, un // replacementIndex - Index of the replacement // // Returns: -// True if the field is in the may-definition set. +// True if the field is in the definition set. // -bool PromotionLiveness::IsReplacementPossiblyDefined(BasicBlock* bb, unsigned structLcl, unsigned replacementIndex) +bool PromotionLiveness::IsReplacementDefined(BasicBlock* bb, unsigned structLcl, unsigned replacementIndex) { unsigned index = m_structLclToTrackedIndex[structLcl] + 1 + replacementIndex; - return BitVecOps::IsMember(m_bvTraits, m_bbInfo[bb->bbNum].VarMayDef, index); + return BitVecOps::IsMember(m_bvTraits, m_bbInfo[bb->bbNum].VarDef, index); } //------------------------------------------------------------------------ diff --git a/src/tests/JIT/Directed/physicalpromotion/physicalpromotion.cs b/src/tests/JIT/Directed/physicalpromotion/physicalpromotion.cs index e01842d91a34b5..e8e914cc4e002a 100644 --- a/src/tests/JIT/Directed/physicalpromotion/physicalpromotion.cs +++ b/src/tests/JIT/Directed/physicalpromotion/physicalpromotion.cs @@ -118,7 +118,8 @@ public static void ReadbacksAcrossBranches(int path, uint expected) S value = new S { A = 17, B = 29 }; Assert.Equal(expected, path is 6 ? ReadbackAfterPartialWrite(value) : ReadbacksAcrossBranchesCore(value, path)); Assert.Equal(path is 0 ? 0U : path is 1 ? 1U : 46U, ReadbackOnlyOnSelectedPath(value, path)); - Assert.Equal(path switch { 0 => 29U, 1 => 63U, 2 => 80U, _ => 46U }, ReadbacksAtLiveJoin(value, path)); + Assert.Equal(path switch { 0 => 29U, 1 => 63U, 2 => 80U, 3 => 70U, 4 => 120U, _ => 46U }, + ReadbacksAtLiveJoin(value, path)); } [MethodImpl(MethodImplOptions.NoInlining)] @@ -138,6 +139,14 @@ private static uint ReadbacksAtLiveJoin(S value, int path) { result = value.A * 2; } + else if (path == 3) + { + value.A = 41; + } + else if (path == 4) + { + value = GetReadbackValue(); + } return result + value.A + value.B; } From 19fefb60407ec9d9a3f5e9d79fc600ef23698f49 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 25 Sep 2026 13:16:25 +0200 Subject: [PATCH 6/7] JIT: Reduce readback planning overhead Cache materialization boundaries for planning and replacement, short-circuit successor traversal, skip planning without eligible incoming fields, and avoid unnecessary definition queries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 628eb6b2-f468-4a67-b2c9-f8de24969633 --- src/coreclr/jit/promotion.cpp | 64 ++++++++++++++++++++++------------- src/coreclr/jit/promotion.h | 1 + 2 files changed, 41 insertions(+), 24 deletions(-) diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index d2cebaa76a7e35..d88ffaa7f6397f 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -1684,12 +1684,19 @@ ReplaceVisitor::ReplaceVisitor(Promotion* prom, , m_dfsTree(dfsTree) , m_postOrderTraits(dfsTree->PostOrderTraits()) { - unsigned index = 0; + unsigned index = 0; + bool hasPlannedReadBackCandidates = false; for (AggregateInfo* agg : m_aggregates) { - for (Replacement& rep : agg->Replacements) + LclVarDsc* dsc = m_compiler->lvaGetDesc(agg->LclNum); + for (unsigned i = 0; i < agg->Replacements.size(); i++) { - rep.ReadBackIndex = index++; + agg->Replacements[i].ReadBackIndex = index++; + if (!hasPlannedReadBackCandidates && (dsc->lvIsParam || dsc->lvIsOSRLocal) && + m_liveness->IsReplacementLiveIn(m_compiler->fgFirstBB, agg->LclNum, i)) + { + hasPlannedReadBackCandidates = true; + } } } @@ -1698,6 +1705,7 @@ ReplaceVisitor::ReplaceVisitor(Promotion* prom, m_currentStructFields = new (m_compiler, CMK_Promotion) BitVec[dfsTree->GetPostOrderCount()]{}; m_processedBlocks = BitVecOps::MakeEmpty(&m_postOrderTraits); m_requiresAlreadyReadBackOnEntry = BitVecOps::MakeEmpty(&m_postOrderTraits); + m_requiresReadBackOnExit = BitVecOps::MakeEmpty(&m_postOrderTraits); for (unsigned i = 0; i < dfsTree->GetPostOrderCount(); i++) { @@ -1717,7 +1725,20 @@ ReplaceVisitor::ReplaceVisitor(Promotion* prom, }); } - PlanReadBacks(); + // The CFG does not change during replacement. Share boundary decisions + // between the planner and the actual materialization. + for (unsigned i = 0; i < dfsTree->GetPostOrderCount(); i++) + { + if (MustMaterializeReadBacks(dfsTree->GetPostOrder(i))) + { + BitVecOps::AddElemD(&m_postOrderTraits, m_requiresReadBackOnExit, i); + } + } + + if (hasPlannedReadBackCandidates) + { + PlanReadBacks(); + } } //------------------------------------------------------------------------ @@ -1738,16 +1759,6 @@ void ReplaceVisitor::PlanReadBacks() FlowGraphDominatorTree* domTree = m_compiler->m_domTree; BitVec pendingOut = BitVecOps::MakeEmpty(&m_postOrderTraits); BitVec sites = BitVecOps::MakeEmpty(&m_postOrderTraits); - BitVec barriers = BitVecOps::MakeEmpty(&m_postOrderTraits); - - for (unsigned i = 0; i < m_dfsTree->GetPostOrderCount(); i++) - { - BasicBlock* block = m_dfsTree->GetPostOrder(i); - if (MustMaterializeReadBacks(block)) - { - BitVecOps::AddElemD(&m_postOrderTraits, barriers, i); - } - } for (AggregateInfo* agg : m_aggregates) { @@ -1814,13 +1825,13 @@ void ReplaceVisitor::PlanReadBacks() BitVecOps::AddElemD(&m_postOrderTraits, sites, block->bbPostorderNum); pending = false; } - if (m_liveness->IsReplacementDefined(block, agg->LclNum, i)) + if (pending && m_liveness->IsReplacementDefined(block, agg->LclNum, i)) { pending = false; } if (pending && m_liveness->IsReplacementLiveOut(block, agg->LclNum, i)) { - if (BitVecOps::IsMember(&m_postOrderTraits, barriers, block->bbPostorderNum)) + if (BitVecOps::IsMember(&m_postOrderTraits, m_requiresReadBackOnExit, block->bbPostorderNum)) { BitVecOps::AddElemD(&m_postOrderTraits, sites, block->bbPostorderNum); } @@ -2034,13 +2045,17 @@ void ReplaceVisitor::InsertReadBackAtEnd(BasicBlock* block, unsigned structLclNu // bool ReplaceVisitor::MustMaterializeReadBacks(BasicBlock* block) { - bool materialize = block->HasPotentialEHSuccs(m_compiler) || - block->KindIs(BBJ_CALLFINALLY, BBJ_EHFINALLYRET, BBJ_EHFILTERRET, BBJ_EHCATCHRET); - block->VisitRegularSuccs(m_compiler, [&](BasicBlock* succ) { - materialize |= BitVecOps::IsMember(&m_postOrderTraits, m_requiresAlreadyReadBackOnEntry, succ->bbPostorderNum); - return BasicBlockVisit::Continue; - }); - return materialize; + if (block->HasPotentialEHSuccs(m_compiler) || + block->KindIs(BBJ_CALLFINALLY, BBJ_EHFINALLYRET, BBJ_EHFILTERRET, BBJ_EHCATCHRET)) + { + return true; + } + + return block->VisitRegularSuccs(m_compiler, [&](BasicBlock* succ) { + return BitVecOps::IsMember(&m_postOrderTraits, m_requiresAlreadyReadBackOnEntry, succ->bbPostorderNum) + ? BasicBlockVisit::Abort + : BasicBlockVisit::Continue; + }) == BasicBlockVisit::Abort; } //------------------------------------------------------------------------ @@ -2053,7 +2068,8 @@ bool ReplaceVisitor::MustMaterializeReadBacks(BasicBlock* block) // void ReplaceVisitor::EndBlock() { - bool materialize = MustMaterializeReadBacks(m_currentBlock); + bool materialize = + BitVecOps::IsMember(&m_postOrderTraits, m_requiresReadBackOnExit, m_currentBlock->bbPostorderNum); BitVec& pendingReadBacks = m_pendingReadBacks[m_currentBlock->bbPostorderNum]; pendingReadBacks = BitVecOps::MakeEmpty(m_readBackTraits); diff --git a/src/coreclr/jit/promotion.h b/src/coreclr/jit/promotion.h index 3ce8123a220aef..89168bca95e890 100644 --- a/src/coreclr/jit/promotion.h +++ b/src/coreclr/jit/promotion.h @@ -259,6 +259,7 @@ class ReplaceVisitor : public GenTreeVisitor BitVec* m_currentStructFields; BitVec m_processedBlocks; BitVec m_requiresAlreadyReadBackOnEntry; + BitVec m_requiresReadBackOnExit; public: enum From c05fd77ce7be82d4b51e984155492712d6c60d66 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 25 Sep 2026 14:47:09 +0200 Subject: [PATCH 7/7] JIT: Separate readback preparation from visitor construction Move field tracking setup, boundary computation, and planning into PrepareReadBacks, called after constructing ReplaceVisitor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 628eb6b2-f468-4a67-b2c9-f8de24969633 --- src/coreclr/jit/promotion.cpp | 17 ++++++++++++++--- src/coreclr/jit/promotion.h | 2 ++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index d88ffaa7f6397f..fa912106f57e5f 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -1665,7 +1665,7 @@ GenTree* Promotion::CreateReadBack(Compiler* compiler, unsigned structLclNum, co //------------------------------------------------------------------------ // ReplaceVisitor: -// Prepare state for propagating pending readbacks in reverse postorder. +// Initialize the replacement visitor. // // Parameters: // prom - Promotion phase. @@ -1684,8 +1684,18 @@ ReplaceVisitor::ReplaceVisitor(Promotion* prom, , m_dfsTree(dfsTree) , m_postOrderTraits(dfsTree->PostOrderTraits()) { - unsigned index = 0; - bool hasPlannedReadBackCandidates = false; +} + +//------------------------------------------------------------------------ +// PrepareReadBacks: +// Initialize field tracking, compute loop/EH materialization boundaries, +// and plan common readbacks before replacing uses. +// +void ReplaceVisitor::PrepareReadBacks() +{ + FlowGraphDfsTree* dfsTree = m_dfsTree; + unsigned index = 0; + bool hasPlannedReadBackCandidates = false; for (AggregateInfo* agg : m_aggregates) { LclVarDsc* dsc = m_compiler->lvaGetDesc(agg->LclNum); @@ -3259,6 +3269,7 @@ PhaseStatus Promotion::Run() FlowGraphDfsTree* dfsTree = m_compiler->m_dfsTree; ReplaceVisitor replacer(this, aggregates, &liveness, dfsTree); + replacer.PrepareReadBacks(); for (unsigned i = dfsTree->GetPostOrderCount(); i > 0; i--) { BasicBlock* bb = dfsTree->GetPostOrder(i - 1); diff --git a/src/coreclr/jit/promotion.h b/src/coreclr/jit/promotion.h index 89168bca95e890..f0111d6d87d0c2 100644 --- a/src/coreclr/jit/promotion.h +++ b/src/coreclr/jit/promotion.h @@ -274,6 +274,8 @@ class ReplaceVisitor : public GenTreeVisitor PromotionLiveness* liveness, FlowGraphDfsTree* dfsTree); + void PrepareReadBacks(); + bool MadeChanges() { return m_madeChanges;