From 2e4f93c226abd8c782ccf7959c916c492f2c5e28 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 6 Jun 2023 13:07:27 +0200 Subject: [PATCH 1/4] JIT: Regularize readbacks for parameters/OSR-locals in physical promotion Handle readbacks for parameters/OSR-locals like any other readback is handled. Previously they were handled by creating the scratch BB and then inserting IR after the main replacement had already been done; now, we instead create the scratch BB eagerly and mark these as requiring a read back at the beginning of the scratch BB, and leave normal replacement logic up to handle it. The main benefit is that this unification makes it easier to ensure that future smarter handling of readbacks/writebacks (e.g. "resolution") automatically kicks in for the common case of parameters. Introduce another invariant, which is that we only ever mark a field as requiring readback if it is live. Previously we would always mark them as requiring read backs, but would then check liveness before inserting the actual IR to do the read back. But we don't always have the liveness information at the point where we insert IR for readbacks after #86706. Also expand some debug logging, and address some feedback from #86706. --- src/coreclr/jit/block.cpp | 2 +- src/coreclr/jit/promotion.cpp | 238 ++++++++++++++------- src/coreclr/jit/promotion.h | 10 +- src/coreclr/jit/promotiondecomposition.cpp | 5 +- src/coreclr/jit/promotionliveness.cpp | 36 +++- 5 files changed, 204 insertions(+), 87 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 7c57df63822c7c..dd6029771265c6 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -896,7 +896,7 @@ BasicBlock* BasicBlock::GetUniquePred(Compiler* compiler) const // // Return Value: // The unique successor of a block, or nullptr if there is no unique successor. - +// BasicBlock* BasicBlock::GetUniqueSucc() const { if (bbJumpKind == BBJ_ALWAYS) diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index f2f32daa9d76c8..88a5f2c6bd6300 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -1150,6 +1150,77 @@ GenTree* Promotion::CreateReadBack(Compiler* compiler, unsigned structLclNum, co return store; } +//------------------------------------------------------------------------ +// StartBlock: +// Handle reaching the end of the currently started block by preparing +// internal state for upcoming basic blocks, and inserting any necessary +// readbacks. +// +// Parameters: +// block - The block +// +void 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. + for (AggregateInfo* agg : m_aggregates) + { + if (agg == nullptr) + { + continue; + } + + for (Replacement& rep : agg->Replacements) + { + assert(!rep.NeedsReadBack); + assert(rep.NeedsWriteBack); + } + } +#endif + + // OSR locals and parameters may need an initial read back, which we mark + // when we start the scratch BB. + if (!m_compiler->fgBBisScratch(block)) + { + return; + } + + for (AggregateInfo* agg : m_aggregates) + { + if (agg == nullptr) + { + continue; + } + + LclVarDsc* dsc = m_compiler->lvaGetDesc(agg->LclNum); + if (!dsc->lvIsParam && !dsc->lvIsOSRLocal) + { + continue; + } + + JITDUMP("Marking fields of %s V%02u as needing read-back in scratch " 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]; + rep.NeedsWriteBack = false; + if (m_liveness->IsReplacementLiveIn(block, agg->LclNum, (unsigned)i)) + { + rep.NeedsReadBack = true; + JITDUMP(" V%02u (%s) marked\n", rep.LclNum, rep.Description); + } + else + { + JITDUMP(" V%02u (%s) not marked (not live-in to scratch BB)\n", rep.LclNum, rep.Description); + } + } + } +} + //------------------------------------------------------------------------ // EndBlock: // Handle reaching the end of the currently started block by preparing @@ -1178,24 +1249,21 @@ void ReplaceVisitor::EndBlock() assert(!rep.NeedsReadBack || !rep.NeedsWriteBack); if (rep.NeedsReadBack) { - 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); - } - else - { - JITDUMP("Skipping reading back dead 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); - } + // We only mark fields as requiring read-back if they are live + // at the point where the stack local was written. If the + // replacement still needs to be read back then we saw no use + // between that point and the end of the BB, so we expect the + // use to be in a successor. + assert(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); rep.NeedsReadBack = false; } @@ -1206,6 +1274,18 @@ void ReplaceVisitor::EndBlock() m_hasPendingReadBacks = false; } +//------------------------------------------------------------------------ +// PostOrderVisit: +// Visit a node in post-order and make necessary changes for promoted field +// uses. +// +// Parameters: +// use - The use edge +// user - The user +// +// Returns: +// Visitor result. +// Compiler::fgWalkResult ReplaceVisitor::PostOrderVisit(GenTree** use, GenTree* user) { GenTree* tree = *use; @@ -1300,16 +1380,13 @@ GenTree** ReplaceVisitor::InsertMidTreeReadBacksIfNecessary(GenTree** use) for (Replacement& rep : agg->Replacements) { - // TODO-CQ: We should ensure we do not mark dead fields as - // requiring readback. Currently it is handled by querying liveness - // as part of end-of-block readback insertion, but for these - // mid-tree readbacks we cannot query liveness information for - // arbitrary locals. if (!rep.NeedsReadBack) { continue; } + JITDUMP(" V%02.[%03u..%03u) -> V%02u\n", agg->LclNum, rep.Offset, genTypeSize(rep.AccessType), rep.LclNum); + rep.NeedsReadBack = false; GenTree* readBack = Promotion::CreateReadBack(m_compiler, agg->LclNum, rep); *use = @@ -1369,10 +1446,7 @@ void ReplaceVisitor::LoadStoreAroundCall(GenTreeCall* call, GenTree* user) GenTreeLclVarCommon* retBufLcl = retBufArg->GetNode()->AsLclVarCommon(); unsigned size = m_compiler->typGetObjLayout(call->gtRetClsHnd)->GetSize(); - if (MarkForReadBack(retBufLcl->GetLclNum(), retBufLcl->GetLclOffs(), size)) - { - JITDUMP("Retbuf has replacements that were marked for read back\n"); - } + MarkForReadBack(retBufLcl, size DEBUGARG("used as retbuf")); } } @@ -1486,9 +1560,9 @@ void ReplaceVisitor::ReplaceLocal(GenTree** use, GenTree* user) Replacement& rep = replacements[index]; assert(accessType == rep.AccessType); - JITDUMP(" ..replaced with promoted lcl V%02u\n", rep.LclNum); bool isDef = lcl->OperIsLocalStore(); + if (isDef) { *use = m_compiler->gtNewStoreLclVarNode(rep.LclNum, lcl->Data()); @@ -1507,6 +1581,7 @@ void ReplaceVisitor::ReplaceLocal(GenTree** use, GenTree* user) } else if (rep.NeedsReadBack) { + JITDUMP(" ..needs a read back\n"); *use = m_compiler->gtNewOperNode(GT_COMMA, (*use)->TypeGet(), Promotion::CreateReadBack(m_compiler, lclNum, rep), *use); rep.NeedsReadBack = false; @@ -1537,6 +1612,8 @@ void ReplaceVisitor::ReplaceLocal(GenTree** use, GenTree* user) m_compiler->lvaGetDesc(rep.LclNum)->lvRedefinedInEmbeddedStatement = true; } + JITDUMP(" ..replaced with V%02u\n", rep.LclNum); + m_madeChanges = true; } @@ -1617,18 +1694,18 @@ void ReplaceVisitor::WriteBackBefore(GenTree** use, unsigned lcl, unsigned offs, // back before their next use. // // Parameters: -// lcl - The struct local -// offs - The starting offset of the range in the struct local that needs to be read back from. -// size - The size of the range +// lcl - Local node. Its offset is the start of the range. +// size - The size of the range // -bool ReplaceVisitor::MarkForReadBack(unsigned lcl, unsigned offs, unsigned size) +void ReplaceVisitor::MarkForReadBack(GenTreeLclVarCommon* lcl, unsigned size DEBUGARG(const char* reason)) { - if (m_aggregates[lcl] == nullptr) + if (m_aggregates[lcl->GetLclNum()] == nullptr) { - return false; + return; } - jitstd::vector& replacements = m_aggregates[lcl]->Replacements; + unsigned offs = lcl->GetLclOffs(); + jitstd::vector& replacements = m_aggregates[lcl->GetLclNum()]->Replacements; size_t index = Promotion::BinarySearch(replacements, offs); if ((ssize_t)index < 0) @@ -1640,20 +1717,37 @@ bool ReplaceVisitor::MarkForReadBack(unsigned lcl, unsigned offs, unsigned size) } } - bool any = false; unsigned end = offs + size; - while ((index < replacements.size()) && (replacements[index].Offset < end)) + if ((index >= replacements.size()) || (replacements[index].Offset >= end)) + { + // No overlap with any field. + return; + } + + StructDeaths deaths = m_liveness->GetDeathsForStructLocal(lcl); + JITDUMP("Fields of [%06u] in range [%03u..%03u) need to be read back: %s\n", Compiler::dspTreeID(lcl), offs, + offs + size, reason); + + do { - any = true; Replacement& rep = replacements[index]; assert(rep.Overlaps(offs, size)); - rep.NeedsReadBack = true; - rep.NeedsWriteBack = false; - m_hasPendingReadBacks = true; - index++; - } - return any; + if (deaths.IsReplacementDying((unsigned)index)) + { + JITDUMP(" V%02u (%s) not marked (is dying)\n", rep.LclNum, rep.Description); + } + else + { + rep.NeedsReadBack = true; + m_hasPendingReadBacks = true; + JITDUMP(" V%02u (%s) marked\n", rep.LclNum, rep.Description); + } + + rep.NeedsWriteBack = false; + + index++; + } while ((index < replacements.size()) && (replacements[index].Offset < end)); } //------------------------------------------------------------------------ @@ -1766,17 +1860,43 @@ PhaseStatus Promotion::Run() return PhaseStatus::MODIFIED_NOTHING; } + // Check for parameters and OSR locals that need to be read back on entry + // to the function. + for (AggregateInfo* agg : aggregates) + { + if (agg == nullptr) + { + continue; + } + + LclVarDsc* dsc = m_compiler->lvaGetDesc(agg->LclNum); + if (dsc->lvIsParam || dsc->lvIsOSRLocal) + { + // We will need an initial readback. We create the scratch BB ahead + // of time so that we get correct liveness and mark the + // parameters/OSR-locals as requiring read-back as part of + // ReplaceVisitor::StartBlock when we get to the scratch block. + m_compiler->fgEnsureFirstBBisScratch(); + break; + } + } + // Compute liveness for the fields and remainders. PromotionLiveness liveness(m_compiler, aggregates); liveness.Run(); JITDUMP("Making replacements\n\n"); + // Make all replacements we decided on. ReplaceVisitor replacer(this, aggregates, &liveness); for (BasicBlock* bb : m_compiler->Blocks()) { replacer.StartBlock(bb); + JITDUMP("\nReplacing in "); + DBEXEC(m_compiler->verbose, bb->dspBlockHeader(m_compiler)); + JITDUMP("\n"); + for (Statement* stmt : bb->Statements()) { DISPSTMT(stmt); @@ -1795,8 +1915,7 @@ PhaseStatus Promotion::Run() replacer.EndBlock(); } - // Insert initial IR to read arguments/OSR locals into replacement locals, - // and add necessary explicit zeroing. + // Add necessary explicit zeroing for some locals. Statement* prevStmt = nullptr; for (unsigned lclNum = 0; lclNum < numLocals; lclNum++) { @@ -1806,11 +1925,7 @@ PhaseStatus Promotion::Run() } LclVarDsc* dsc = m_compiler->lvaGetDesc(lclNum); - if (dsc->lvIsParam || dsc->lvIsOSRLocal) - { - InsertInitialReadBack(lclNum, aggregates[lclNum]->Replacements, &prevStmt); - } - else if (dsc->lvSuppressedZeroInit) + if (dsc->lvSuppressedZeroInit) { // We may have suppressed inserting an explicit zero init based on the // assumption that the entire local will be zero inited in the prolog. @@ -1856,27 +1971,6 @@ bool Promotion::IsCandidateForPhysicalPromotion(LclVarDsc* dsc) return (dsc->TypeGet() == TYP_STRUCT) && !dsc->lvPromoted && !dsc->IsAddressExposed(); } -//------------------------------------------------------------------------ -// Promotion::InsertInitialReadBack: -// Insert IR to initially read a struct local's value into its promoted field locals. -// -// Parameters: -// lclNum - The struct local -// replacements - Replacements for the struct local -// prevStmt - [in, out] Previous statement to insert after -// -void Promotion::InsertInitialReadBack(unsigned lclNum, - const jitstd::vector& replacements, - Statement** prevStmt) -{ - for (unsigned i = 0; i < replacements.size(); i++) - { - const Replacement& rep = replacements[i]; - GenTree* readBack = CreateReadBack(m_compiler, lclNum, rep); - InsertInitStatement(prevStmt, readBack); - } -} - //------------------------------------------------------------------------ // Promotion::ExplicitlyZeroInitReplacementLocals: // Insert IR to zero out replacement locals if necessary. diff --git a/src/coreclr/jit/promotion.h b/src/coreclr/jit/promotion.h index 24ebffe722e7ea..2c4a2c57c426e1 100644 --- a/src/coreclr/jit/promotion.h +++ b/src/coreclr/jit/promotion.h @@ -118,7 +118,6 @@ class Promotion static StructSegments SignificantSegments(Compiler* compiler, ClassLayout* layout DEBUGARG(FixedBitVect** bitVectRepr = nullptr)); - void InsertInitialReadBack(unsigned lclNum, const jitstd::vector& replacements, Statement** prevStmt); void ExplicitlyZeroInitReplacementLocals(unsigned lclNum, const jitstd::vector& replacements, Statement** prevStmt); @@ -226,6 +225,7 @@ class PromotionLiveness } void Run(); + bool IsReplacementLiveIn(BasicBlock* bb, unsigned structLcl, unsigned replacement); bool IsReplacementLiveOut(BasicBlock* bb, unsigned structLcl, unsigned replacement); StructDeaths GetDeathsForStructLocal(GenTreeLclVarCommon* use); @@ -271,11 +271,7 @@ class ReplaceVisitor : public GenTreeVisitor return m_madeChanges; } - void StartBlock(BasicBlock* block) - { - m_currentBlock = block; - } - + void StartBlock(BasicBlock* block); void EndBlock(); void StartStatement() @@ -292,7 +288,7 @@ class ReplaceVisitor : public GenTreeVisitor void ReplaceLocal(GenTree** use, GenTree* user); void StoreBeforeReturn(GenTreeUnOp* ret); void WriteBackBefore(GenTree** use, unsigned lcl, unsigned offs, unsigned size); - bool MarkForReadBack(unsigned lcl, unsigned offs, unsigned size); + void MarkForReadBack(GenTreeLclVarCommon* lcl, unsigned size DEBUGARG(const char* reason)); void HandleStore(GenTree** use, GenTree* user); bool OverlappingReplacements(GenTreeLclVarCommon* lcl, diff --git a/src/coreclr/jit/promotiondecomposition.cpp b/src/coreclr/jit/promotiondecomposition.cpp index d0df8b9e17fa62..023da0dc60a2a4 100644 --- a/src/coreclr/jit/promotiondecomposition.cpp +++ b/src/coreclr/jit/promotiondecomposition.cpp @@ -1124,10 +1124,7 @@ void ReplaceVisitor::HandleStore(GenTree** use, GenTree* user) { GenTreeLclVarCommon* lclStore = store->AsLclVarCommon(); unsigned size = lclStore->GetLayout(m_compiler)->GetSize(); - if (MarkForReadBack(lclStore->GetLclNum(), lclStore->GetLclOffs(), size)) - { - JITDUMP("Marked store destination replacements to be read back (could not decompose this store)\n"); - } + MarkForReadBack(lclStore, size DEBUGARG("cannot decompose store")); } } } diff --git a/src/coreclr/jit/promotionliveness.cpp b/src/coreclr/jit/promotionliveness.cpp index 71ac5a96b3b8d1..613db4bcd56dcf 100644 --- a/src/coreclr/jit/promotionliveness.cpp +++ b/src/coreclr/jit/promotionliveness.cpp @@ -688,8 +688,19 @@ void PromotionLiveness::FillInLiveness(BitVec& life, BitVec volatileVars, GenTre if (lcl->OperIs(GT_LCL_ADDR)) { // Retbuf -- these are definitions but we do not know of how much. - // We never mark them as dead and we never treat them as killing anything. - assert(isDef); + // We never treat them as killing anything, but we do store liveness information for them. + BitVecTraits aggTraits(1 + (unsigned)agg->Replacements.size(), m_compiler); + BitVec aggDeaths(BitVecOps::MakeEmpty(&aggTraits)); + // Copy preexisting liveness information. + for (size_t i = 0; i <= agg->Replacements.size(); i++) + { + unsigned varIndex = baseIndex + (unsigned)i; + if (!BitVecOps::IsMember(m_bvTraits, life, varIndex)) + { + BitVecOps::AddElemD(&aggTraits, aggDeaths, (unsigned)i); + } + } + m_aggDeaths.Set(lcl, aggDeaths); return; } @@ -747,6 +758,24 @@ void PromotionLiveness::FillInLiveness(BitVec& life, BitVec volatileVars, GenTre } } +//------------------------------------------------------------------------ +// IsReplacementLiveIn: +// Check if a replacement field is live at the start of a basic block. +// +// Parameters: +// structLcl - The struct (base) local +// replacementIndex - Index of the replacement +// +// Returns: +// True if the field is in the live-in set. +// +bool PromotionLiveness::IsReplacementLiveIn(BasicBlock* bb, unsigned structLcl, unsigned replacementIndex) +{ + BitVec liveIn = m_bbInfo[bb->bbNum].LiveIn; + unsigned baseIndex = m_structLclToTrackedIndex[structLcl]; + return BitVecOps::IsMember(m_bvTraits, liveIn, baseIndex + 1 + replacementIndex); +} + //------------------------------------------------------------------------ // IsReplacementLiveOut: // Check if a replacement field is live at the end of a basic block. @@ -778,7 +807,8 @@ bool PromotionLiveness::IsReplacementLiveOut(BasicBlock* bb, unsigned structLcl, // StructDeaths PromotionLiveness::GetDeathsForStructLocal(GenTreeLclVarCommon* lcl) { - assert(lcl->OperIsLocal() && lcl->TypeIs(TYP_STRUCT) && (m_aggregates[lcl->GetLclNum()] != nullptr)); + assert((lcl->TypeIs(TYP_STRUCT) || (lcl->OperIs(GT_LCL_ADDR) && ((lcl->gtFlags & GTF_VAR_DEF) != 0))) && + (m_aggregates[lcl->GetLclNum()] != nullptr)); BitVec aggDeaths; bool found = m_aggDeaths.Lookup(lcl, &aggDeaths); assert(found); From 28c9ec70042761df5e1a58a8c1fa64f9acf8413d Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 6 Jun 2023 13:22:36 +0200 Subject: [PATCH 2/4] Unrelated parameter type fix --- src/coreclr/jit/promotionliveness.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/promotionliveness.cpp b/src/coreclr/jit/promotionliveness.cpp index 613db4bcd56dcf..43b37226144893 100644 --- a/src/coreclr/jit/promotionliveness.cpp +++ b/src/coreclr/jit/promotionliveness.cpp @@ -190,7 +190,7 @@ void PromotionLiveness::ComputeUseDefSets() // useSet - The use set to mark in. // defSet - The def set to mark in. // -void PromotionLiveness::MarkUseDef(GenTreeLclVarCommon* lcl, BitSetShortLongRep& useSet, BitSetShortLongRep& defSet) +void PromotionLiveness::MarkUseDef(GenTreeLclVarCommon* lcl, BitVec& useSet, BitVec& defSet) { AggregateInfo* agg = m_aggregates[lcl->GetLclNum()]; if (agg == nullptr) From b001c6022438d8d633ae8ca212161b63a9817fd6 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 6 Jun 2023 15:25:44 +0200 Subject: [PATCH 3/4] Fix a bad assumption --- src/coreclr/jit/promotion.cpp | 49 ++++++++++++++++++++++++----------- 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index 88a5f2c6bd6300..a5516b9cbfa5c3 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -1249,21 +1249,40 @@ void ReplaceVisitor::EndBlock() assert(!rep.NeedsReadBack || !rep.NeedsWriteBack); if (rep.NeedsReadBack) { - // We only mark fields as requiring read-back if they are live - // at the point where the stack local was written. If the - // replacement still needs to be read back then we saw no use - // between that point and the end of the BB, so we expect the - // use to be in a successor. - assert(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 (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); + } + else + { + // We only mark fields as requiring read-back if they are + // live at the point where the stack local was written, so + // at first glance we would not expect this case to ever + // happen. However, it is possible that the field is live + // because it has a future struct use, in which case we may + // not need to insert any readbacks anywhere. For example, + // consider: + // + // V03 = CALL() // V03 is a struct with promoted V03.[000..008) + // CALL(struct V03) // V03.[000.008) marked as live here + // + // While V03.[000.008) gets marked for readback at the + // assignment, no readback is necessary at the location of + // the call argument, and it may die after that. + + JITDUMP("Skipping reading back dead 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); + } + rep.NeedsReadBack = false; } From a169410b6b792c0ff4ce5986754bffbe1f5ae56e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sat, 17 Jun 2023 11:38:12 +0200 Subject: [PATCH 4/4] Nit --- src/coreclr/jit/promotion.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/promotion.cpp b/src/coreclr/jit/promotion.cpp index 3dd3968892c3c6..ee2ba01e1bb0af 100644 --- a/src/coreclr/jit/promotion.cpp +++ b/src/coreclr/jit/promotion.cpp @@ -2368,8 +2368,9 @@ void ReplaceVisitor::WriteBackBefore(GenTree** use, unsigned lcl, unsigned offs, // back before their next use. // // Parameters: -// lcl - Local node. Its offset is the start of the range. -// size - The size of the range +// lcl - Local node. Its offset is the start of the range. +// size - The size of the range +// reason - The reason the readback is required // void ReplaceVisitor::MarkForReadBack(GenTreeLclVarCommon* lcl, unsigned size DEBUGARG(const char* reason)) {