From 2e96b632dc313e2421b04b9500f1d453aa0757c3 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sat, 12 Aug 2023 00:28:48 +0200 Subject: [PATCH 01/12] JIT: Add a head merging pass Add a pass that does head merging to compliment the existing tail merging pass. Unlike tail merging this requires reordering the first statement with the terminator node of the predecessor, which requires some interference checking. Fix #90017 --- src/coreclr/jit/block.h | 5 + src/coreclr/jit/compiler.cpp | 4 +- src/coreclr/jit/compiler.h | 3 +- src/coreclr/jit/fgopt.cpp | 260 ++++++++++++++++++++++++++++++++++- src/coreclr/jit/fgstmt.cpp | 2 +- 5 files changed, 269 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index cbdcd66af8590..21b37f17fc456 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -722,6 +722,11 @@ struct BasicBlock : private LIR::Range return KindIs(kind) || KindIs(rest...); } + bool HasTerminator() + { + return KindIs(BBJ_EHFINALLYRET, BBJ_EHFAULTRET, BBJ_EHFILTERRET, BBJ_COND, BBJ_SWITCH, BBJ_RETURN); + } + // NumSucc() gives the number of successors, and GetSucc() returns a given numbered successor. // // There are two versions of these functions: ones that take a Compiler* and ones that don't. You must diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index a04bd56057abb..44975eae92469 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4718,7 +4718,7 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl { // Tail merge // - DoPhase(this, PHASE_TAIL_MERGE, &Compiler::fgTailMerge); + DoPhase(this, PHASE_TAIL_MERGE, [this]() { return fgTailMerge(true); }); // Merge common throw blocks // @@ -4835,7 +4835,7 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // Second pass of tail merge // - DoPhase(this, PHASE_TAIL_MERGE2, &Compiler::fgTailMerge); + DoPhase(this, PHASE_TAIL_MERGE2, [this]() { return fgTailMerge(false); }); // Compute reachability sets and dominators. // diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 3214a8a581c82..0a3a67e2d140d 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5519,7 +5519,8 @@ class Compiler void fgMoveBlocksAfter(BasicBlock* bStart, BasicBlock* bEnd, BasicBlock* insertAfterBlk); - PhaseStatus fgTailMerge(); + PhaseStatus fgTailMerge(bool early); + bool fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, BasicBlock* pred); enum FG_RELOCATE_TYPE { diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 480739ea69337..947093d1f60c5 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -6657,6 +6657,121 @@ void Compiler::fgCompDominatedByExceptionalEntryBlocks() } } +bool Compiler::fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, BasicBlock* pred) +{ + if (!pred->HasTerminator()) + { + return true; + } + + GenTree* tree1 = pred->lastStmt()->GetRootNode(); + GenTree* tree2 = firstStmt->GetRootNode(); + + GenTreeFlags tree1Flags = tree1->gtFlags; + GenTreeFlags tree2Flags = tree2->gtFlags; + + if (early) + { + tree1Flags |= gtHasLocalsWithAddrOp(tree1) ? GTF_GLOB_REF : GTF_EMPTY; + tree2Flags |= gtHasLocalsWithAddrOp(tree2) ? GTF_GLOB_REF : GTF_EMPTY; + } + + // We do not support embedded statements in the terminator node. + if ((tree1Flags & GTF_ASG) != 0) + { + JITDUMP(" no; terminator contains embedded store\n"); + return false; + } + if ((tree2Flags & GTF_ASG) != 0) + { + // Handle common case where the second statement is a top-level store. + if (!tree2->OperIsLocalStore()) + { + JITDUMP(" cannot reorder with GTF_ASG without top-level store"); + return false; + } + + GenTreeLclVarCommon* lcl = tree2->AsLclVarCommon(); + if ((lcl->Data()->gtFlags & GTF_ASG) != 0) + { + JITDUMP(" cannot reorder with embedded store"); + return false; + } + + LclVarDsc* dsc = lvaGetDesc(tree2->AsLclVarCommon()); + if ((tree1Flags & GTF_ALL_EFFECT) != 0) + { + if (early ? dsc->lvHasLdAddrOp : dsc->IsAddressExposed()) + { + JITDUMP(" cannot reorder store to exposed local with any side effect\n"); + return false; + } + } + + if (gtHasRef(tree1, lcl->GetLclNum())) + { + JITDUMP(" cannot reorder with interfering use\n"); + return false; + } + + if (dsc->lvIsStructField && gtHasRef(tree1, dsc->lvParentLcl)) + { + JITDUMP(" cannot reorder with interferring use of parent struct local\n"); + return false; + } + + if (dsc->lvPromoted) + { + for (int i = 0; i < dsc->lvFieldCnt; i++) + { + if (gtHasRef(tree1, dsc->lvFieldLclStart + i)) + { + JITDUMP(" cannot reorder with interferring use of struct field\n"); + return false; + } + } + } + + // We've validated that the store does not interfere. Get rid of the + // flag for the future checks. + tree2Flags &= ~GTF_ASG; + } + + if (((tree1Flags & GTF_CALL) != 0) && ((tree2Flags & GTF_ALL_EFFECT) != 0)) + { + JITDUMP(" cannot reorder call with any side effect\n"); + return false; + } + if (((tree1Flags & GTF_GLOB_REF) != 0) && ((tree2Flags & GTF_PERSISTENT_SIDE_EFFECTS) != 0)) + { + JITDUMP(" cannot reorder global reference with persistent side effects\n"); + return false; + } + if ((tree1Flags & GTF_ORDER_SIDEEFF) != 0) + { + if ((tree2Flags & (GTF_GLOB_REF | GTF_ORDER_SIDEEFF)) != 0) + { + JITDUMP(" cannot reorder ordering side effect\n"); + return false; + } + } + if ((tree2Flags & GTF_ORDER_SIDEEFF) != 0) + { + if ((tree1Flags & (GTF_GLOB_REF | GTF_ORDER_SIDEEFF)) != 0) + { + JITDUMP(" cannot reorder ordering side effect\n"); + return false; + } + } + if (((tree1Flags & GTF_EXCEPT) != 0) && ((tree2Flags & GTF_SIDE_EFFECT) != 0)) + { + JITDUMP(" cannot reorder exception with side effect\n"); + return false; + } + + return true; +} + //------------------------------------------------------------------------ // fgTailMerge: merge common sequences of statements in block predecessors // @@ -6680,7 +6795,7 @@ void Compiler::fgCompDominatedByExceptionalEntryBlocks() // incurring too much TP overhead. It's possible to make the merging // more efficient and if so it might be worth revising this value. // -PhaseStatus Compiler::fgTailMerge() +PhaseStatus Compiler::fgTailMerge(bool early) { bool madeChanges = false; int const mergeLimit = 50; @@ -7031,6 +7146,149 @@ PhaseStatus Compiler::fgTailMerge() iterateTailMerge(retryBlocks.Pop()); } + // Try head merging a block. + // If return value is true, retry. + // May also add to retryBlocks. + // + auto headMerge = [&](BasicBlock* block) -> bool { + + if (block->NumSucc(this) < 2) + { + // Nothing to merge here + return false; + } + + matchedPredInfo.Reset(); + + // Find the subset of preds that reach along non-critical edges + // and populate predInfo. + // + for (BasicBlock* const succBlock : block->Succs(this)) + { + if (succBlock->GetUniquePred(this) != block) + { + return false; + } + + if (!BasicBlock::sameEHRegion(block, succBlock)) + { + return false; + } + + Statement* firstStmt = nullptr; + // Walk past any GT_NOPs. + // + for (Statement* stmt : succBlock->Statements()) + { + if (stmt->GetRootNode()->OperIs(GT_NOP)) + { + continue; + } + + firstStmt = stmt; + break; + } + + // Block might be effectively empty. + // + if (firstStmt == nullptr) + { + return false; + } + + // Cannot move terminator statement. + // + if ((firstStmt == succBlock->lastStmt()) && succBlock->HasTerminator()) + { + return false; + } + + if (firstStmt->GetRootNode()->IsCall() && firstStmt->GetRootNode()->AsCall()->CanTailCall()) + { + return false; + } + + if ((matchedPredInfo.Height() == 0) || + GenTree::Compare(firstStmt->GetRootNode(), matchedPredInfo.BottomRef(0).m_stmt->GetRootNode())) + { + matchedPredInfo.Emplace(succBlock, firstStmt); + } + else + { + return false; + } + } + + Statement* firstStmt = matchedPredInfo.TopRef(0).m_stmt; + JITDUMP("All succs of " FMT_BB " start with the same tree\n", block->bbNum); + DISPSTMT(firstStmt); + + JITDUMP("Checking if we can move it into the predecessor...\n"); + + if (!fgCanMoveFirstStatementIntoPred(early, firstStmt, block)) + { + return false; + } + + JITDUMP("Moving statement\n"); + + for (int i = 0; i < matchedPredInfo.Height(); i++) + { + PredInfo& info = matchedPredInfo.TopRef(i); + Statement* const stmt = info.m_stmt; + BasicBlock* const succBlock = info.m_block; + + fgUnlinkStmt(succBlock, stmt); + + // Add one of the matching stmts to block, and + // update its flags. + // + if (i == 0) + { + fgInsertStmtNearEnd(block, stmt); + block->bbFlags |= succBlock->bbFlags & BBF_COPY_PROPAGATE; + } + + madeChanges = true; + } + + return true; + }; + + auto iterateHeadMerge = [&](BasicBlock* block) -> void { + + int numOpts = 0; + bool retry = true; + + while (retry) + { + retry = headMerge(block); + if (retry) + { + numOpts++; + } + } + + if (numOpts > 0) + { + JITDUMP("Did %d head merges in " FMT_BB "\n", numOpts, block->bbNum); + } + }; + + // Visit each block + // + for (BasicBlock* const block : Blocks()) + { + iterateHeadMerge(block); + } + + // Work through any retries + // + while (retryBlocks.Height() > 0) + { + iterateHeadMerge(retryBlocks.Pop()); + } + // If we altered flow, reset fgModified. Given where we sit in the // phase list, flow-dependent side data hasn't been built yet, so // nothing needs invalidation. diff --git a/src/coreclr/jit/fgstmt.cpp b/src/coreclr/jit/fgstmt.cpp index 4651acbdfb247..fead5b82e0b34 100644 --- a/src/coreclr/jit/fgstmt.cpp +++ b/src/coreclr/jit/fgstmt.cpp @@ -182,7 +182,7 @@ void Compiler::fgInsertStmtNearEnd(BasicBlock* block, Statement* stmt) // This routine can only be used when in tree order. assert(fgOrder == FGOrderTree); - if (block->KindIs(BBJ_EHFINALLYRET, BBJ_EHFAULTRET, BBJ_EHFILTERRET, BBJ_COND, BBJ_SWITCH, BBJ_RETURN)) + if (block->HasTerminator()) { Statement* firstStmt = block->firstStmt(); noway_assert(firstStmt != nullptr); From e7e6962b2a2c471b8d650b2fc57f7b71524557e8 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sat, 12 Aug 2023 21:25:23 +0200 Subject: [PATCH 02/12] Remove dead code --- src/coreclr/jit/fgopt.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 947093d1f60c5..1d5a37007fc7e 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -7282,13 +7282,6 @@ PhaseStatus Compiler::fgTailMerge(bool early) iterateHeadMerge(block); } - // Work through any retries - // - while (retryBlocks.Height() > 0) - { - iterateHeadMerge(retryBlocks.Pop()); - } - // If we altered flow, reset fgModified. Given where we sit in the // phase list, flow-dependent side data hasn't been built yet, so // nothing needs invalidation. From a5bc9d034e3f3c294ac98c98e024bafa8f42d881 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 16 Aug 2023 12:34:49 +0200 Subject: [PATCH 03/12] Add docs, clean up names --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/fgopt.cpp | 102 ++++++++++++++++++++++--------------- 2 files changed, 63 insertions(+), 41 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 0a3a67e2d140d..bb13d599de4a3 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5519,7 +5519,7 @@ class Compiler void fgMoveBlocksAfter(BasicBlock* bStart, BasicBlock* bEnd, BasicBlock* insertAfterBlk); - PhaseStatus fgTailMerge(bool early); + PhaseStatus fgHeadTailMerge(bool early); bool fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, BasicBlock* pred); enum FG_RELOCATE_TYPE diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 1d5a37007fc7e..dee33091f4ba4 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -6657,6 +6657,21 @@ void Compiler::fgCompDominatedByExceptionalEntryBlocks() } } +//------------------------------------------------------------------------ +// fgCanMoveFirstStatementIntoPred: Check if the first statement of a block can +// be moved into its predecessor. +// +// Parameters: +// early - Whether this is being checked with early IR invariants (where +// we do not have valid address exposure/GTF_GLOB_REF). +// firstStmt - The statement to move +// pred - The predecessor block +// +// Remarks: +// Unlike tail merging, for head merging we have to either spill the +// predecessor's terminator node, or reorder it with the head statement. +// Here we choose to reorder. +// bool Compiler::fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, BasicBlock* pred) { if (!pred->HasTerminator()) @@ -6773,14 +6788,15 @@ bool Compiler::fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, } //------------------------------------------------------------------------ -// fgTailMerge: merge common sequences of statements in block predecessors +// fgHeadTailMerge: merge common sequences of statements in block predecessors/successors // // Returns: // Suitable phase status. // // Notes: -// Looks for cases where all or some predecessors of a block have the same -// (or equivalent) last statement. +// This applies tail merging and head merging. For tail merging it looks for +// cases where all or some predecessors of a block have the same (or +// equivalent) last statement. // // If all predecessors have the same last statement, move one of them to // the start of the block, and delete the copies in the preds. @@ -6791,11 +6807,16 @@ bool Compiler::fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, // the canonical, and delete the copies in the cross jump blocks. // Then retry merging on the canonical block. // +// Conversely, for head merging, we look for cases where all successors of a +// block start with the same statement. We then try to move one of them into +// the predecessor (which requires special handling due to the terminator +// node) and delete the copies. +// // We set a mergeLimit to try and get most of the benefit while not // incurring too much TP overhead. It's possible to make the merging // more efficient and if so it might be worth revising this value. // -PhaseStatus Compiler::fgTailMerge(bool early) +PhaseStatus Compiler::fgHeadTailMerge(bool early) { bool madeChanges = false; int const mergeLimit = 50; @@ -6822,18 +6843,18 @@ PhaseStatus Compiler::fgTailMerge(bool early) return PhaseStatus::MODIFIED_NOTHING; } - struct PredInfo + struct PredSuccInfo { - PredInfo(BasicBlock* block, Statement* stmt) : m_block(block), m_stmt(stmt) + PredSuccInfo(BasicBlock* block, Statement* stmt) : m_block(block), m_stmt(stmt) { } BasicBlock* m_block; Statement* m_stmt; }; - ArrayStack predInfo(getAllocator(CMK_ArrayStack)); - ArrayStack matchedPredInfo(getAllocator(CMK_ArrayStack)); - ArrayStack retryBlocks(getAllocator(CMK_ArrayStack)); + ArrayStack predSuccInfo(getAllocator(CMK_ArrayStack)); + ArrayStack matchedPredSuccInfo(getAllocator(CMK_ArrayStack)); + ArrayStack retryBlocks(getAllocator(CMK_ArrayStack)); // Try tail merging a block. // If return value is true, retry. @@ -6847,10 +6868,10 @@ PhaseStatus Compiler::fgTailMerge(bool early) return false; } - predInfo.Reset(); + predSuccInfo.Reset(); // Find the subset of preds that reach along non-critical edges - // and populate predInfo. + // and populate predSuccInfo. // for (BasicBlock* const predBlock : block->PredBlocks()) { @@ -6899,12 +6920,12 @@ PhaseStatus Compiler::fgTailMerge(bool early) // We don't expect to see PHIs but watch for them anyways. // assert(!lastStmt->IsPhiDefnStmt()); - predInfo.Emplace(predBlock, lastStmt); + predSuccInfo.Emplace(predBlock, lastStmt); } // Are there enough preds to make it interesting? // - if (predInfo.Height() < 2) + if (predSuccInfo.Height() < 2) { // Not enough preds to merge return false; @@ -6916,7 +6937,7 @@ PhaseStatus Compiler::fgTailMerge(bool early) // Note we check this rather than countOfInEdges because we don't care // about dups, just the number of unique pred blocks. // - if (predInfo.Height() > mergeLimit) + if (predSuccInfo.Height() > mergeLimit) { // Too many preds to consider return false; @@ -6925,24 +6946,24 @@ PhaseStatus Compiler::fgTailMerge(bool early) // Find a matching set of preds. Potentially O(N^2) tree comparisons. // int i = 0; - while (i < (predInfo.Height() - 1)) + while (i < (predSuccInfo.Height() - 1)) { - matchedPredInfo.Reset(); - matchedPredInfo.Emplace(predInfo.TopRef(i)); - Statement* const baseStmt = predInfo.TopRef(i).m_stmt; - for (int j = i + 1; j < predInfo.Height(); j++) + matchedPredSuccInfo.Reset(); + matchedPredSuccInfo.Emplace(predSuccInfo.TopRef(i)); + Statement* const baseStmt = predSuccInfo.TopRef(i).m_stmt; + for (int j = i + 1; j < predSuccInfo.Height(); j++) { - Statement* const otherStmt = predInfo.TopRef(j).m_stmt; + Statement* const otherStmt = predSuccInfo.TopRef(j).m_stmt; // Consider: compute and cache hashes to make this faster // if (GenTree::Compare(baseStmt->GetRootNode(), otherStmt->GetRootNode())) { - matchedPredInfo.Emplace(predInfo.TopRef(j)); + matchedPredSuccInfo.Emplace(predSuccInfo.TopRef(j)); } } - if (matchedPredInfo.Height() < 2) + if (matchedPredSuccInfo.Height() < 2) { // This pred didn't match any other. Check other preds for matches. i++; @@ -6952,14 +6973,14 @@ PhaseStatus Compiler::fgTailMerge(bool early) // We have some number of preds that have identical last statements. // If all preds of block have a matching last stmt, move that statement to the start of block. // - if (matchedPredInfo.Height() == (int)block->countOfInEdges()) + if (matchedPredSuccInfo.Height() == (int)block->countOfInEdges()) { JITDUMP("All preds of " FMT_BB " end with the same tree, moving\n", block->bbNum); - JITDUMPEXEC(gtDispStmt(matchedPredInfo.TopRef(0).m_stmt)); + JITDUMPEXEC(gtDispStmt(matchedPredSuccInfo.TopRef(0).m_stmt)); - for (int j = 0; j < matchedPredInfo.Height(); j++) + for (int j = 0; j < matchedPredSuccInfo.Height(); j++) { - PredInfo& info = matchedPredInfo.TopRef(j); + PredSuccInfo& info = matchedPredSuccInfo.TopRef(j); Statement* const stmt = info.m_stmt; BasicBlock* const predBlock = info.m_block; @@ -6986,17 +7007,18 @@ PhaseStatus Compiler::fgTailMerge(bool early) // Pick one block as the victim -- preferably a block with just one // statement or one that falls through to block (or both). // - JITDUMP("A set of %d preds of " FMT_BB " end with the same tree\n", matchedPredInfo.Height(), block->bbNum); - JITDUMPEXEC(gtDispStmt(matchedPredInfo.TopRef(0).m_stmt)); + JITDUMP("A set of %d preds of " FMT_BB " end with the same tree\n", matchedPredSuccInfo.Height(), + block->bbNum); + JITDUMPEXEC(gtDispStmt(matchedPredSuccInfo.TopRef(0).m_stmt)); BasicBlock* crossJumpVictim = nullptr; Statement* crossJumpStmt = nullptr; bool haveNoSplitVictim = false; bool haveFallThroughVictim = false; - for (int j = 0; j < matchedPredInfo.Height(); j++) + for (int j = 0; j < matchedPredSuccInfo.Height(); j++) { - PredInfo& info = matchedPredInfo.TopRef(j); + PredSuccInfo& info = matchedPredSuccInfo.TopRef(j); Statement* const stmt = info.m_stmt; BasicBlock* const predBlock = info.m_block; @@ -7069,9 +7091,9 @@ PhaseStatus Compiler::fgTailMerge(bool early) // Do the cross jumping // - for (int j = 0; j < matchedPredInfo.Height(); j++) + for (int j = 0; j < matchedPredSuccInfo.Height(); j++) { - PredInfo& info = matchedPredInfo.TopRef(j); + PredSuccInfo& info = matchedPredSuccInfo.TopRef(j); BasicBlock* const predBlock = info.m_block; Statement* const stmt = info.m_stmt; @@ -7158,10 +7180,10 @@ PhaseStatus Compiler::fgTailMerge(bool early) return false; } - matchedPredInfo.Reset(); + matchedPredSuccInfo.Reset(); // Find the subset of preds that reach along non-critical edges - // and populate predInfo. + // and populate predSuccInfo. // for (BasicBlock* const succBlock : block->Succs(this)) { @@ -7208,10 +7230,10 @@ PhaseStatus Compiler::fgTailMerge(bool early) return false; } - if ((matchedPredInfo.Height() == 0) || - GenTree::Compare(firstStmt->GetRootNode(), matchedPredInfo.BottomRef(0).m_stmt->GetRootNode())) + if ((matchedPredSuccInfo.Height() == 0) || + GenTree::Compare(firstStmt->GetRootNode(), matchedPredSuccInfo.BottomRef(0).m_stmt->GetRootNode())) { - matchedPredInfo.Emplace(succBlock, firstStmt); + matchedPredSuccInfo.Emplace(succBlock, firstStmt); } else { @@ -7219,7 +7241,7 @@ PhaseStatus Compiler::fgTailMerge(bool early) } } - Statement* firstStmt = matchedPredInfo.TopRef(0).m_stmt; + Statement* firstStmt = matchedPredSuccInfo.TopRef(0).m_stmt; JITDUMP("All succs of " FMT_BB " start with the same tree\n", block->bbNum); DISPSTMT(firstStmt); @@ -7232,9 +7254,9 @@ PhaseStatus Compiler::fgTailMerge(bool early) JITDUMP("Moving statement\n"); - for (int i = 0; i < matchedPredInfo.Height(); i++) + for (int i = 0; i < matchedPredSuccInfo.Height(); i++) { - PredInfo& info = matchedPredInfo.TopRef(i); + PredSuccInfo& info = matchedPredSuccInfo.TopRef(i); Statement* const stmt = info.m_stmt; BasicBlock* const succBlock = info.m_block; From adfd827f60cb596b52b1b9a191ca8d0277d43bbf Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 16 Aug 2023 12:50:18 +0200 Subject: [PATCH 04/12] Further renaming --- src/coreclr/jit/compiler.cpp | 4 ++-- src/coreclr/jit/compphases.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 44975eae92469..4b5079ea4bfa1 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4718,7 +4718,7 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl { // Tail merge // - DoPhase(this, PHASE_TAIL_MERGE, [this]() { return fgTailMerge(true); }); + DoPhase(this, PHASE_HEAD_TAIL_MERGE, [this]() { return fgHeadTailMerge(true); }); // Merge common throw blocks // @@ -4835,7 +4835,7 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // Second pass of tail merge // - DoPhase(this, PHASE_TAIL_MERGE2, [this]() { return fgTailMerge(false); }); + DoPhase(this, PHASE_HEAD_TAIL_MERGE2, [this]() { return fgHeadTailMerge(false); }); // Compute reachability sets and dominators. // diff --git a/src/coreclr/jit/compphases.h b/src/coreclr/jit/compphases.h index 9fe31ca846410..9ee73c7d69ad6 100644 --- a/src/coreclr/jit/compphases.h +++ b/src/coreclr/jit/compphases.h @@ -55,10 +55,10 @@ CompPhaseNameMacro(PHASE_COMPUTE_EDGE_WEIGHTS, "Compute edge weights (1, f #if defined(FEATURE_EH_FUNCLETS) CompPhaseNameMacro(PHASE_CREATE_FUNCLETS, "Create EH funclets", false, -1, false) #endif // FEATURE_EH_FUNCLETS -CompPhaseNameMacro(PHASE_TAIL_MERGE, "Tail merge", false, -1, false) +CompPhaseNameMacro(PHASE_HEAD_TAIL_MERGE, "Head and tail merge", false, -1, false) CompPhaseNameMacro(PHASE_MERGE_THROWS, "Merge throw blocks", false, -1, false) CompPhaseNameMacro(PHASE_INVERT_LOOPS, "Invert loops", false, -1, false) -CompPhaseNameMacro(PHASE_TAIL_MERGE2, "Post-morph tail merge", false, -1, false) +CompPhaseNameMacro(PHASE_HEAD_TAIL_MERGE2, "Post-morph head and tail merge", false, -1, false) CompPhaseNameMacro(PHASE_OPTIMIZE_FLOW, "Optimize control flow", false, -1, false) CompPhaseNameMacro(PHASE_OPTIMIZE_LAYOUT, "Optimize layout", false, -1, false) CompPhaseNameMacro(PHASE_COMPUTE_REACHABILITY, "Compute blocks reachability", false, -1, false) From 291a8946546fcabc89c7ae720e9eca095de40e2b Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 17 Aug 2023 15:12:32 +0200 Subject: [PATCH 05/12] Clean up, address some feedback --- src/coreclr/jit/compiler.h | 2 + src/coreclr/jit/fgopt.cpp | 255 ++++++++++++++++++++----------------- 2 files changed, 139 insertions(+), 118 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index bb13d599de4a3..b117a366735c2 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5520,6 +5520,8 @@ class Compiler void fgMoveBlocksAfter(BasicBlock* bStart, BasicBlock* bEnd, BasicBlock* insertAfterBlk); PhaseStatus fgHeadTailMerge(bool early); + bool fgTryOneHeadMerge(BasicBlock* block, ArrayStack& matchedPredSuccInfo, bool early); + bool fgHeadMerge(BasicBlock* block, ArrayStack& matchedPredSuccInfo, bool early); bool fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, BasicBlock* pred); enum FG_RELOCATE_TYPE diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index dee33091f4ba4..91cd726e733ee 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -6674,6 +6674,40 @@ void Compiler::fgCompDominatedByExceptionalEntryBlocks() // bool Compiler::fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, BasicBlock* pred) { + struct HasTailCallCandidateVisitor : GenTreeVisitor + { + enum + { + DoPreOrder = true + }; + + HasTailCallCandidateVisitor(Compiler* comp) : GenTreeVisitor(comp) + { + } + + fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) + { + GenTree* node = *use; + if ((node->gtFlags & GTF_CALL) == 0) + { + return WALK_SKIP_SUBTREES; + } + + if (node->IsCall() && node->AsCall()->CanTailCall()) + { + return WALK_ABORT; + } + + return WALK_CONTINUE; + } + }; + + HasTailCallCandidateVisitor visitor(this); + if (visitor.WalkTree(firstStmt->GetRootNodePointer(), nullptr) == WALK_ABORT) + { + return false; + } + if (!pred->HasTerminator()) { return true; @@ -6731,7 +6765,7 @@ bool Compiler::fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, if (dsc->lvIsStructField && gtHasRef(tree1, dsc->lvParentLcl)) { - JITDUMP(" cannot reorder with interferring use of parent struct local\n"); + JITDUMP(" cannot reorder with interfering use of parent struct local\n"); return false; } @@ -6741,7 +6775,7 @@ bool Compiler::fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, { if (gtHasRef(tree1, dsc->lvFieldLclStart + i)) { - JITDUMP(" cannot reorder with interferring use of struct field\n"); + JITDUMP(" cannot reorder with interfering use of struct field\n"); return false; } } @@ -6787,6 +6821,15 @@ bool Compiler::fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, return true; } +struct PredSuccInfo +{ + PredSuccInfo(BasicBlock* block, Statement* stmt) : m_block(block), m_stmt(stmt) + { + } + BasicBlock* m_block; + Statement* m_stmt; +}; + //------------------------------------------------------------------------ // fgHeadTailMerge: merge common sequences of statements in block predecessors/successors // @@ -6843,15 +6886,6 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) return PhaseStatus::MODIFIED_NOTHING; } - struct PredSuccInfo - { - PredSuccInfo(BasicBlock* block, Statement* stmt) : m_block(block), m_stmt(stmt) - { - } - BasicBlock* m_block; - Statement* m_stmt; - }; - ArrayStack predSuccInfo(getAllocator(CMK_ArrayStack)); ArrayStack matchedPredSuccInfo(getAllocator(CMK_ArrayStack)); ArrayStack retryBlocks(getAllocator(CMK_ArrayStack)); @@ -7168,147 +7202,132 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) iterateTailMerge(retryBlocks.Pop()); } - // Try head merging a block. - // If return value is true, retry. - // May also add to retryBlocks. + // Visit each block + // + for (BasicBlock* const block : Blocks()) + { + madeChanges |= fgHeadMerge(block, matchedPredSuccInfo, early); + } + + // If we altered flow, reset fgModified. Given where we sit in the + // phase list, flow-dependent side data hasn't been built yet, so + // nothing needs invalidation. // - auto headMerge = [&](BasicBlock* block) -> bool { + fgModified = false; - if (block->NumSucc(this) < 2) + return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; +} + +bool Compiler::fgTryOneHeadMerge(BasicBlock* block, ArrayStack& matchedPredSuccInfo, bool early) +{ + if (block->NumSucc(this) < 2) + { + // Nothing to merge here + return false; + } + + matchedPredSuccInfo.Reset(); + + // Find the subset of preds that reach along non-critical edges + // and populate predSuccInfo. + // + for (BasicBlock* const succBlock : block->Succs(this)) + { + if (succBlock->GetUniquePred(this) != block) { - // Nothing to merge here return false; } - matchedPredSuccInfo.Reset(); + if (!BasicBlock::sameEHRegion(block, succBlock)) + { + return false; + } - // Find the subset of preds that reach along non-critical edges - // and populate predSuccInfo. + Statement* firstStmt = nullptr; + // Walk past any GT_NOPs. // - for (BasicBlock* const succBlock : block->Succs(this)) + for (Statement* stmt : succBlock->Statements()) { - if (succBlock->GetUniquePred(this) != block) - { - return false; - } - - if (!BasicBlock::sameEHRegion(block, succBlock)) - { - return false; - } - - Statement* firstStmt = nullptr; - // Walk past any GT_NOPs. - // - for (Statement* stmt : succBlock->Statements()) + if (!stmt->GetRootNode()->OperIs(GT_NOP)) { - if (stmt->GetRootNode()->OperIs(GT_NOP)) - { - continue; - } - firstStmt = stmt; break; } - - // Block might be effectively empty. - // - if (firstStmt == nullptr) - { - return false; - } - - // Cannot move terminator statement. - // - if ((firstStmt == succBlock->lastStmt()) && succBlock->HasTerminator()) - { - return false; - } - - if (firstStmt->GetRootNode()->IsCall() && firstStmt->GetRootNode()->AsCall()->CanTailCall()) - { - return false; - } - - if ((matchedPredSuccInfo.Height() == 0) || - GenTree::Compare(firstStmt->GetRootNode(), matchedPredSuccInfo.BottomRef(0).m_stmt->GetRootNode())) - { - matchedPredSuccInfo.Emplace(succBlock, firstStmt); - } - else - { - return false; - } } - Statement* firstStmt = matchedPredSuccInfo.TopRef(0).m_stmt; - JITDUMP("All succs of " FMT_BB " start with the same tree\n", block->bbNum); - DISPSTMT(firstStmt); - - JITDUMP("Checking if we can move it into the predecessor...\n"); - - if (!fgCanMoveFirstStatementIntoPred(early, firstStmt, block)) + // Block might be effectively empty. + // + if (firstStmt == nullptr) { return false; } - JITDUMP("Moving statement\n"); + // Cannot move terminator statement. + // + if ((firstStmt == succBlock->lastStmt()) && succBlock->HasTerminator()) + { + return false; + } - for (int i = 0; i < matchedPredSuccInfo.Height(); i++) + if ((matchedPredSuccInfo.Height() == 0) || + GenTree::Compare(firstStmt->GetRootNode(), matchedPredSuccInfo.BottomRef(0).m_stmt->GetRootNode())) + { + matchedPredSuccInfo.Emplace(succBlock, firstStmt); + } + else { - PredSuccInfo& info = matchedPredSuccInfo.TopRef(i); - Statement* const stmt = info.m_stmt; - BasicBlock* const succBlock = info.m_block; + return false; + } + } - fgUnlinkStmt(succBlock, stmt); + Statement* firstStmt = matchedPredSuccInfo.TopRef(0).m_stmt; + JITDUMP("All succs of " FMT_BB " start with the same tree\n", block->bbNum); + DISPSTMT(firstStmt); - // Add one of the matching stmts to block, and - // update its flags. - // - if (i == 0) - { - fgInsertStmtNearEnd(block, stmt); - block->bbFlags |= succBlock->bbFlags & BBF_COPY_PROPAGATE; - } + JITDUMP("Checking if we can move it into the predecessor...\n"); - madeChanges = true; - } + if (!fgCanMoveFirstStatementIntoPred(early, firstStmt, block)) + { + return false; + } - return true; - }; + JITDUMP("Moving statement\n"); - auto iterateHeadMerge = [&](BasicBlock* block) -> void { + for (int i = 0; i < matchedPredSuccInfo.Height(); i++) + { + PredSuccInfo& info = matchedPredSuccInfo.TopRef(i); + Statement* const stmt = info.m_stmt; + BasicBlock* const succBlock = info.m_block; - int numOpts = 0; - bool retry = true; + fgUnlinkStmt(succBlock, stmt); - while (retry) + // Add one of the matching stmts to block, and + // update its flags. + // + if (i == 0) { - retry = headMerge(block); - if (retry) - { - numOpts++; - } + fgInsertStmtNearEnd(block, stmt); + block->bbFlags |= succBlock->bbFlags & BBF_COPY_PROPAGATE; } + } - if (numOpts > 0) - { - JITDUMP("Did %d head merges in " FMT_BB "\n", numOpts, block->bbNum); - } - }; + return true; +} - // Visit each block - // - for (BasicBlock* const block : Blocks()) +bool Compiler::fgHeadMerge(BasicBlock* block, ArrayStack& matchedPredSuccInfo, bool early) +{ + bool madeChanges = false; + int numOpts = 0; + while (fgTryOneHeadMerge(block, matchedPredSuccInfo, early)) { - iterateHeadMerge(block); + madeChanges = true; + numOpts++; } - // If we altered flow, reset fgModified. Given where we sit in the - // phase list, flow-dependent side data hasn't been built yet, so - // nothing needs invalidation. - // - fgModified = false; + if (numOpts > 0) + { + JITDUMP("Did %d head merges in " FMT_BB "\n", numOpts, block->bbNum); + } - return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; + return madeChanges; } From df52773560538dc8523a02f3bb8661fb656bef24 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 17 Aug 2023 17:00:15 +0200 Subject: [PATCH 06/12] Rename some more stuff; fix a bug; add a test for the bug --- src/coreclr/jit/compiler.hpp | 2 + src/coreclr/jit/fgopt.cpp | 39 +++++++++---------- src/coreclr/jit/jitconfigvalues.h | 6 +-- .../opt/HeadTailMerge/headmergeexception.cs | 37 ++++++++++++++++++ .../HeadTailMerge/headmergeexception.csproj | 8 ++++ 5 files changed, 68 insertions(+), 24 deletions(-) create mode 100644 src/tests/JIT/opt/HeadTailMerge/headmergeexception.cs create mode 100644 src/tests/JIT/opt/HeadTailMerge/headmergeexception.csproj diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index ae9e180dbe83d..1699b36c7c2c4 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -676,6 +676,8 @@ inline bool BasicBlock::HasPotentialEHSuccs(Compiler* comp) return false; } + // Throwing in a filter is the same as returning "continue search", which + // causes enclosed finally/fault handlers to be executed. return hndDesc->InFilterRegionBBRange(this); } diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 91cd726e733ee..e47af26931689 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -6755,6 +6755,12 @@ bool Compiler::fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, JITDUMP(" cannot reorder store to exposed local with any side effect\n"); return false; } + + if (((tree1Flags & (GTF_CALL | GTF_EXCEPT)) != 0) && pred->HasPotentialEHSuccs(this)) + { + JITDUMP(" cannot reorder assignment with exception throwing tree and potential EH successor\n"); + return false; + } } if (gtHasRef(tree1, lcl->GetLclNum())) @@ -6864,27 +6870,23 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) bool madeChanges = false; int const mergeLimit = 50; - const bool isEnabled = JitConfig.JitEnableTailMerge() > 0; + const bool isEnabled = JitConfig.JitEnableHeadTailMerge() > 0; if (!isEnabled) { - JITDUMP("Tail merge disabled by JitEnableTailMerge\n"); + JITDUMP("Head and tail merge disabled by JitEnableHeadTailMerge\n"); return PhaseStatus::MODIFIED_NOTHING; } #ifdef DEBUG - static ConfigMethodRange JitEnableTailMergeRange; - JitEnableTailMergeRange.EnsureInit(JitConfig.JitEnableTailMergeRange()); - const unsigned hash = impInlineRoot()->info.compMethodHash(); - const bool inRange = JitEnableTailMergeRange.Contains(hash); -#else - const bool inRange = true; -#endif - - if (!inRange) + static ConfigMethodRange JitEnableHeadTailMergeRange; + JitEnableHeadTailMergeRange.EnsureInit(JitConfig.JitEnableHeadTailMergeRange()); + const unsigned hash = impInlineRoot()->info.compMethodHash(); + if (!JitEnableHeadTailMergeRange.Contains(hash)) { - JITDUMP("Tail merge disabled by JitEnableTailMergeRange\n"); + JITDUMP("Tail merge disabled by JitEnableHeadTailMergeRange\n"); return PhaseStatus::MODIFIED_NOTHING; } +#endif ArrayStack predSuccInfo(getAllocator(CMK_ArrayStack)); ArrayStack matchedPredSuccInfo(getAllocator(CMK_ArrayStack)); @@ -7170,16 +7172,11 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) auto iterateTailMerge = [&](BasicBlock* block) -> void { - int numOpts = 0; - bool retry = true; + int numOpts = 0; - while (retry) + while (tailMerge(block)) { - retry = tailMerge(block); - if (retry) - { - numOpts++; - } + numOpts++; } if (numOpts > 0) @@ -7317,7 +7314,7 @@ bool Compiler::fgTryOneHeadMerge(BasicBlock* block, ArrayStack& ma bool Compiler::fgHeadMerge(BasicBlock* block, ArrayStack& matchedPredSuccInfo, bool early) { bool madeChanges = false; - int numOpts = 0; + int numOpts = 0; while (fgTryOneHeadMerge(block, matchedPredSuccInfo, early)) { madeChanges = true; diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index b3384031f355e..ae01210a3934c 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -427,7 +427,7 @@ CONFIG_INTEGER(JitDoVNBasedDeadStoreRemoval, W("JitDoVNBasedDeadStoreRemoval"), // removal CONFIG_INTEGER(JitDoRedundantBranchOpts, W("JitDoRedundantBranchOpts"), 1) // Perform redundant branch optimizations CONFIG_STRING(JitEnableRboRange, W("JitEnableRboRange")) -CONFIG_STRING(JitEnableTailMergeRange, W("JitEnableTailMergeRange")) +CONFIG_STRING(JitEnableHeadTailMergeRange, W("JitEnableHeadTailMergeRange")) CONFIG_STRING(JitEnableVNBasedDeadStoreRemovalRange, W("JitEnableVNBasedDeadStoreRemovalRange")) CONFIG_STRING(JitEnableEarlyLivenessRange, W("JitEnableEarlyLivenessRange")) CONFIG_STRING(JitOnlyOptimizeRange, @@ -621,8 +621,8 @@ CONFIG_INTEGER(JitForceControlFlowGuard, W("JitForceControlFlowGuard"), 0); // 2: Default behavior, depends on platform (yes on x64, no on arm64) CONFIG_INTEGER(JitCFGUseDispatcher, W("JitCFGUseDispatcher"), 2) -// Enable tail merging -CONFIG_INTEGER(JitEnableTailMerge, W("JitEnableTailMerge"), 1) +// Enable head and tail merging +CONFIG_INTEGER(JitEnableHeadTailMerge, W("JitEnableHeadTailMerge"), 1) // Enable physical promotion CONFIG_INTEGER(JitEnablePhysicalPromotion, W("JitEnablePhysicalPromotion"), 1) diff --git a/src/tests/JIT/opt/HeadTailMerge/headmergeexception.cs b/src/tests/JIT/opt/HeadTailMerge/headmergeexception.cs new file mode 100644 index 0000000000000..607f3f04e3c4f --- /dev/null +++ b/src/tests/JIT/opt/HeadTailMerge/headmergeexception.cs @@ -0,0 +1,37 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; +using Xunit; + +public class HeadMergeException +{ + [Fact] + public static int TestEntryPoint() + { + int local = 100; + try + { + // Ensure head merging does not move the write to 'local' out here. + if (Throws()) + { + local = local + 1; + local *= 2; + } + else + { + local = local + 1; + local *= 3; + } + } + catch (Exception) + { + } + + return local; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool Throws() => throw new Exception(); +} diff --git a/src/tests/JIT/opt/HeadTailMerge/headmergeexception.csproj b/src/tests/JIT/opt/HeadTailMerge/headmergeexception.csproj new file mode 100644 index 0000000000000..de6d5e08882e8 --- /dev/null +++ b/src/tests/JIT/opt/HeadTailMerge/headmergeexception.csproj @@ -0,0 +1,8 @@ + + + True + + + + + From cd6d07502663e7d6121526540a9b9597de6749fe Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 17 Aug 2023 17:02:10 +0200 Subject: [PATCH 07/12] Fix indentation --- .../opt/HeadTailMerge/headmergeexception.cs | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/tests/JIT/opt/HeadTailMerge/headmergeexception.cs b/src/tests/JIT/opt/HeadTailMerge/headmergeexception.cs index 607f3f04e3c4f..b6b72a0d1be02 100644 --- a/src/tests/JIT/opt/HeadTailMerge/headmergeexception.cs +++ b/src/tests/JIT/opt/HeadTailMerge/headmergeexception.cs @@ -10,28 +10,28 @@ public class HeadMergeException [Fact] public static int TestEntryPoint() { - int local = 100; - try - { + int local = 100; + try + { // Ensure head merging does not move the write to 'local' out here. - if (Throws()) - { - local = local + 1; - local *= 2; - } - else - { - local = local + 1; - local *= 3; - } - } - catch (Exception) - { - } + if (Throws()) + { + local = local + 1; + local *= 2; + } + else + { + local = local + 1; + local *= 3; + } + } + catch (Exception) + { + } - return local; + return local; } - [MethodImpl(MethodImplOptions.NoInlining)] - private static bool Throws() => throw new Exception(); + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool Throws() => throw new Exception(); } From 1b0b236c68f210f415d694856ee298b3ce1e7d19 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 17 Aug 2023 17:04:38 +0200 Subject: [PATCH 08/12] Terminology --- src/coreclr/jit/fgopt.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index e47af26931689..7c7a8b5bb6ca5 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -6758,7 +6758,7 @@ bool Compiler::fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, if (((tree1Flags & (GTF_CALL | GTF_EXCEPT)) != 0) && pred->HasPotentialEHSuccs(this)) { - JITDUMP(" cannot reorder assignment with exception throwing tree and potential EH successor\n"); + JITDUMP(" cannot reorder store with exception throwing tree and potential EH successor\n"); return false; } } From 22ba1cef8f393c25c74e2cdf715899bb8d8f23e8 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 17 Aug 2023 17:17:14 +0200 Subject: [PATCH 09/12] Write function headers --- src/coreclr/jit/fgopt.cpp | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 7c7a8b5bb6ca5..722a92ebf9388 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -6839,6 +6839,10 @@ struct PredSuccInfo //------------------------------------------------------------------------ // fgHeadTailMerge: merge common sequences of statements in block predecessors/successors // +// Parameters: +// early - Whether this is being checked with early IR invariants (where +// we do not have valid address exposure/GTF_GLOB_REF). +// // Returns: // Suitable phase status. // @@ -7215,6 +7219,19 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } +//------------------------------------------------------------------------ +// fgTryOneHeadMerge: Try to merge the first statement of the successors of a +// specified block. +// +// Parameters: +// block - The block whose successors are to be considered +// matchedPredSuccInfo - Storage data structure +// early - Whether this is being checked with early IR invariants +// (where we do not have valid address exposure/GTF_GLOB_REF). +// +// Returns: +// True if the merge succeeded. +// bool Compiler::fgTryOneHeadMerge(BasicBlock* block, ArrayStack& matchedPredSuccInfo, bool early) { if (block->NumSucc(this) < 2) @@ -7311,6 +7328,19 @@ bool Compiler::fgTryOneHeadMerge(BasicBlock* block, ArrayStack& ma return true; } +//------------------------------------------------------------------------ +// fgHeadMerge: Try to repeatedly merge the first statement of the successors +// of the specified block. +// +// Parameters: +// block - The block whose successors are to be considered +// matchedPredSuccInfo - Storage data structure +// early - Whether this is being checked with early IR invariants +// (where we do not have valid address exposure/GTF_GLOB_REF). +// +// Returns: +// True if any merge succeeded. +// bool Compiler::fgHeadMerge(BasicBlock* block, ArrayStack& matchedPredSuccInfo, bool early) { bool madeChanges = false; From 2bcbbeb9bc631ce478f3d161e4102b08f2b6db16 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 17 Aug 2023 21:28:08 +0200 Subject: [PATCH 10/12] Fix test --- src/tests/JIT/Directed/debugging/debuginfo/tester.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/JIT/Directed/debugging/debuginfo/tester.csproj b/src/tests/JIT/Directed/debugging/debuginfo/tester.csproj index f6cfe3f42f585..bdc02aa4f6d70 100644 --- a/src/tests/JIT/Directed/debugging/debuginfo/tester.csproj +++ b/src/tests/JIT/Directed/debugging/debuginfo/tester.csproj @@ -12,7 +12,7 @@ - + From ed2279f1642eed8f47902d603ee4f0db7d77bb07 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 17 Aug 2023 22:12:02 +0200 Subject: [PATCH 11/12] Revise to BBJ_COND; fix tailcall checks --- src/coreclr/jit/compiler.h | 5 +- src/coreclr/jit/fgopt.cpp | 526 +++++++++++++++++++------------------ 2 files changed, 269 insertions(+), 262 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index b117a366735c2..92d67b08e4061 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5520,8 +5520,9 @@ class Compiler void fgMoveBlocksAfter(BasicBlock* bStart, BasicBlock* bEnd, BasicBlock* insertAfterBlk); PhaseStatus fgHeadTailMerge(bool early); - bool fgTryOneHeadMerge(BasicBlock* block, ArrayStack& matchedPredSuccInfo, bool early); - bool fgHeadMerge(BasicBlock* block, ArrayStack& matchedPredSuccInfo, bool early); + bool fgHeadMerge(BasicBlock* block, bool early); + bool fgTryOneHeadMerge(BasicBlock* block, bool early); + bool gtTreeContainsTailCall(GenTree* tree); bool fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, BasicBlock* pred); enum FG_RELOCATE_TYPE diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 722a92ebf9388..9e03366a14ff5 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -6657,185 +6657,6 @@ void Compiler::fgCompDominatedByExceptionalEntryBlocks() } } -//------------------------------------------------------------------------ -// fgCanMoveFirstStatementIntoPred: Check if the first statement of a block can -// be moved into its predecessor. -// -// Parameters: -// early - Whether this is being checked with early IR invariants (where -// we do not have valid address exposure/GTF_GLOB_REF). -// firstStmt - The statement to move -// pred - The predecessor block -// -// Remarks: -// Unlike tail merging, for head merging we have to either spill the -// predecessor's terminator node, or reorder it with the head statement. -// Here we choose to reorder. -// -bool Compiler::fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, BasicBlock* pred) -{ - struct HasTailCallCandidateVisitor : GenTreeVisitor - { - enum - { - DoPreOrder = true - }; - - HasTailCallCandidateVisitor(Compiler* comp) : GenTreeVisitor(comp) - { - } - - fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) - { - GenTree* node = *use; - if ((node->gtFlags & GTF_CALL) == 0) - { - return WALK_SKIP_SUBTREES; - } - - if (node->IsCall() && node->AsCall()->CanTailCall()) - { - return WALK_ABORT; - } - - return WALK_CONTINUE; - } - }; - - HasTailCallCandidateVisitor visitor(this); - if (visitor.WalkTree(firstStmt->GetRootNodePointer(), nullptr) == WALK_ABORT) - { - return false; - } - - if (!pred->HasTerminator()) - { - return true; - } - - GenTree* tree1 = pred->lastStmt()->GetRootNode(); - GenTree* tree2 = firstStmt->GetRootNode(); - - GenTreeFlags tree1Flags = tree1->gtFlags; - GenTreeFlags tree2Flags = tree2->gtFlags; - - if (early) - { - tree1Flags |= gtHasLocalsWithAddrOp(tree1) ? GTF_GLOB_REF : GTF_EMPTY; - tree2Flags |= gtHasLocalsWithAddrOp(tree2) ? GTF_GLOB_REF : GTF_EMPTY; - } - - // We do not support embedded statements in the terminator node. - if ((tree1Flags & GTF_ASG) != 0) - { - JITDUMP(" no; terminator contains embedded store\n"); - return false; - } - if ((tree2Flags & GTF_ASG) != 0) - { - // Handle common case where the second statement is a top-level store. - if (!tree2->OperIsLocalStore()) - { - JITDUMP(" cannot reorder with GTF_ASG without top-level store"); - return false; - } - - GenTreeLclVarCommon* lcl = tree2->AsLclVarCommon(); - if ((lcl->Data()->gtFlags & GTF_ASG) != 0) - { - JITDUMP(" cannot reorder with embedded store"); - return false; - } - - LclVarDsc* dsc = lvaGetDesc(tree2->AsLclVarCommon()); - if ((tree1Flags & GTF_ALL_EFFECT) != 0) - { - if (early ? dsc->lvHasLdAddrOp : dsc->IsAddressExposed()) - { - JITDUMP(" cannot reorder store to exposed local with any side effect\n"); - return false; - } - - if (((tree1Flags & (GTF_CALL | GTF_EXCEPT)) != 0) && pred->HasPotentialEHSuccs(this)) - { - JITDUMP(" cannot reorder store with exception throwing tree and potential EH successor\n"); - return false; - } - } - - if (gtHasRef(tree1, lcl->GetLclNum())) - { - JITDUMP(" cannot reorder with interfering use\n"); - return false; - } - - if (dsc->lvIsStructField && gtHasRef(tree1, dsc->lvParentLcl)) - { - JITDUMP(" cannot reorder with interfering use of parent struct local\n"); - return false; - } - - if (dsc->lvPromoted) - { - for (int i = 0; i < dsc->lvFieldCnt; i++) - { - if (gtHasRef(tree1, dsc->lvFieldLclStart + i)) - { - JITDUMP(" cannot reorder with interfering use of struct field\n"); - return false; - } - } - } - - // We've validated that the store does not interfere. Get rid of the - // flag for the future checks. - tree2Flags &= ~GTF_ASG; - } - - if (((tree1Flags & GTF_CALL) != 0) && ((tree2Flags & GTF_ALL_EFFECT) != 0)) - { - JITDUMP(" cannot reorder call with any side effect\n"); - return false; - } - if (((tree1Flags & GTF_GLOB_REF) != 0) && ((tree2Flags & GTF_PERSISTENT_SIDE_EFFECTS) != 0)) - { - JITDUMP(" cannot reorder global reference with persistent side effects\n"); - return false; - } - if ((tree1Flags & GTF_ORDER_SIDEEFF) != 0) - { - if ((tree2Flags & (GTF_GLOB_REF | GTF_ORDER_SIDEEFF)) != 0) - { - JITDUMP(" cannot reorder ordering side effect\n"); - return false; - } - } - if ((tree2Flags & GTF_ORDER_SIDEEFF) != 0) - { - if ((tree1Flags & (GTF_GLOB_REF | GTF_ORDER_SIDEEFF)) != 0) - { - JITDUMP(" cannot reorder ordering side effect\n"); - return false; - } - } - if (((tree1Flags & GTF_EXCEPT) != 0) && ((tree2Flags & GTF_SIDE_EFFECT) != 0)) - { - JITDUMP(" cannot reorder exception with side effect\n"); - return false; - } - - return true; -} - -struct PredSuccInfo -{ - PredSuccInfo(BasicBlock* block, Statement* stmt) : m_block(block), m_stmt(stmt) - { - } - BasicBlock* m_block; - Statement* m_stmt; -}; - //------------------------------------------------------------------------ // fgHeadTailMerge: merge common sequences of statements in block predecessors/successors // @@ -6892,9 +6713,18 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) } #endif - ArrayStack predSuccInfo(getAllocator(CMK_ArrayStack)); - ArrayStack matchedPredSuccInfo(getAllocator(CMK_ArrayStack)); - ArrayStack retryBlocks(getAllocator(CMK_ArrayStack)); + struct PredInfo + { + PredInfo(BasicBlock* block, Statement* stmt) : m_block(block), m_stmt(stmt) + { + } + BasicBlock* m_block; + Statement* m_stmt; + }; + + ArrayStack predInfo(getAllocator(CMK_ArrayStack)); + ArrayStack matchedPredInfo(getAllocator(CMK_ArrayStack)); + ArrayStack retryBlocks(getAllocator(CMK_ArrayStack)); // Try tail merging a block. // If return value is true, retry. @@ -6908,7 +6738,7 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) return false; } - predSuccInfo.Reset(); + predInfo.Reset(); // Find the subset of preds that reach along non-critical edges // and populate predSuccInfo. @@ -6960,12 +6790,12 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) // We don't expect to see PHIs but watch for them anyways. // assert(!lastStmt->IsPhiDefnStmt()); - predSuccInfo.Emplace(predBlock, lastStmt); + predInfo.Emplace(predBlock, lastStmt); } // Are there enough preds to make it interesting? // - if (predSuccInfo.Height() < 2) + if (predInfo.Height() < 2) { // Not enough preds to merge return false; @@ -6977,7 +6807,7 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) // Note we check this rather than countOfInEdges because we don't care // about dups, just the number of unique pred blocks. // - if (predSuccInfo.Height() > mergeLimit) + if (predInfo.Height() > mergeLimit) { // Too many preds to consider return false; @@ -6986,24 +6816,24 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) // Find a matching set of preds. Potentially O(N^2) tree comparisons. // int i = 0; - while (i < (predSuccInfo.Height() - 1)) + while (i < (predInfo.Height() - 1)) { - matchedPredSuccInfo.Reset(); - matchedPredSuccInfo.Emplace(predSuccInfo.TopRef(i)); - Statement* const baseStmt = predSuccInfo.TopRef(i).m_stmt; - for (int j = i + 1; j < predSuccInfo.Height(); j++) + matchedPredInfo.Reset(); + matchedPredInfo.Emplace(predInfo.TopRef(i)); + Statement* const baseStmt = predInfo.TopRef(i).m_stmt; + for (int j = i + 1; j < predInfo.Height(); j++) { - Statement* const otherStmt = predSuccInfo.TopRef(j).m_stmt; + Statement* const otherStmt = predInfo.TopRef(j).m_stmt; // Consider: compute and cache hashes to make this faster // if (GenTree::Compare(baseStmt->GetRootNode(), otherStmt->GetRootNode())) { - matchedPredSuccInfo.Emplace(predSuccInfo.TopRef(j)); + matchedPredInfo.Emplace(predInfo.TopRef(j)); } } - if (matchedPredSuccInfo.Height() < 2) + if (matchedPredInfo.Height() < 2) { // This pred didn't match any other. Check other preds for matches. i++; @@ -7013,14 +6843,14 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) // We have some number of preds that have identical last statements. // If all preds of block have a matching last stmt, move that statement to the start of block. // - if (matchedPredSuccInfo.Height() == (int)block->countOfInEdges()) + if (matchedPredInfo.Height() == (int)block->countOfInEdges()) { JITDUMP("All preds of " FMT_BB " end with the same tree, moving\n", block->bbNum); - JITDUMPEXEC(gtDispStmt(matchedPredSuccInfo.TopRef(0).m_stmt)); + JITDUMPEXEC(gtDispStmt(matchedPredInfo.TopRef(0).m_stmt)); - for (int j = 0; j < matchedPredSuccInfo.Height(); j++) + for (int j = 0; j < matchedPredInfo.Height(); j++) { - PredSuccInfo& info = matchedPredSuccInfo.TopRef(j); + PredInfo& info = matchedPredInfo.TopRef(j); Statement* const stmt = info.m_stmt; BasicBlock* const predBlock = info.m_block; @@ -7047,18 +6877,17 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) // Pick one block as the victim -- preferably a block with just one // statement or one that falls through to block (or both). // - JITDUMP("A set of %d preds of " FMT_BB " end with the same tree\n", matchedPredSuccInfo.Height(), - block->bbNum); - JITDUMPEXEC(gtDispStmt(matchedPredSuccInfo.TopRef(0).m_stmt)); + JITDUMP("A set of %d preds of " FMT_BB " end with the same tree\n", matchedPredInfo.Height(), block->bbNum); + JITDUMPEXEC(gtDispStmt(matchedPredInfo.TopRef(0).m_stmt)); BasicBlock* crossJumpVictim = nullptr; Statement* crossJumpStmt = nullptr; bool haveNoSplitVictim = false; bool haveFallThroughVictim = false; - for (int j = 0; j < matchedPredSuccInfo.Height(); j++) + for (int j = 0; j < matchedPredInfo.Height(); j++) { - PredSuccInfo& info = matchedPredSuccInfo.TopRef(j); + PredInfo& info = matchedPredInfo.TopRef(j); Statement* const stmt = info.m_stmt; BasicBlock* const predBlock = info.m_block; @@ -7131,9 +6960,9 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) // Do the cross jumping // - for (int j = 0; j < matchedPredSuccInfo.Height(); j++) + for (int j = 0; j < matchedPredInfo.Height(); j++) { - PredSuccInfo& info = matchedPredSuccInfo.TopRef(j); + PredInfo& info = matchedPredInfo.TopRef(j); BasicBlock* const predBlock = info.m_block; Statement* const stmt = info.m_stmt; @@ -7203,11 +7032,11 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) iterateTailMerge(retryBlocks.Pop()); } - // Visit each block + // Visit each block and try to merge first statements of successors. // for (BasicBlock* const block : Blocks()) { - madeChanges |= fgHeadMerge(block, matchedPredSuccInfo, early); + madeChanges |= fgHeadMerge(block, early); } // If we altered flow, reset fgModified. Given where we sit in the @@ -7224,107 +7053,102 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) // specified block. // // Parameters: -// block - The block whose successors are to be considered -// matchedPredSuccInfo - Storage data structure -// early - Whether this is being checked with early IR invariants -// (where we do not have valid address exposure/GTF_GLOB_REF). +// block - The block whose successors are to be considered +// early - Whether this is being checked with early IR invariants +// (where we do not have valid address exposure/GTF_GLOB_REF). // // Returns: // True if the merge succeeded. // -bool Compiler::fgTryOneHeadMerge(BasicBlock* block, ArrayStack& matchedPredSuccInfo, bool early) +bool Compiler::fgTryOneHeadMerge(BasicBlock* block, bool early) { - if (block->NumSucc(this) < 2) + // We currently only check for BBJ_COND, which gets the common case of + // spill clique created stores by the importer (often produced due to + // ternaries in C#). + // The logic below could be generalized to BBJ_SWITCH, but this currently + // has almost no CQ benefit but does have a TP impact. + if ((block->bbJumpKind != BBJ_COND) || (block->bbNext == block->bbJumpDest)) { - // Nothing to merge here return false; } - matchedPredSuccInfo.Reset(); - - // Find the subset of preds that reach along non-critical edges - // and populate predSuccInfo. - // - for (BasicBlock* const succBlock : block->Succs(this)) - { - if (succBlock->GetUniquePred(this) != block) + // Verify that both successors are reached along non-critical edges. + auto getSuccCandidate = [=](BasicBlock* succ, Statement** firstStmt) -> bool { + if (succ->GetUniquePred(this) != block) { return false; } - if (!BasicBlock::sameEHRegion(block, succBlock)) + if (!BasicBlock::sameEHRegion(block, succ)) { return false; } - Statement* firstStmt = nullptr; + *firstStmt = nullptr; // Walk past any GT_NOPs. // - for (Statement* stmt : succBlock->Statements()) + for (Statement* stmt : succ->Statements()) { if (!stmt->GetRootNode()->OperIs(GT_NOP)) { - firstStmt = stmt; + *firstStmt = stmt; break; } } // Block might be effectively empty. // - if (firstStmt == nullptr) + if (*firstStmt == nullptr) { return false; } // Cannot move terminator statement. // - if ((firstStmt == succBlock->lastStmt()) && succBlock->HasTerminator()) + if ((*firstStmt == succ->lastStmt()) && succ->HasTerminator()) { return false; } - if ((matchedPredSuccInfo.Height() == 0) || - GenTree::Compare(firstStmt->GetRootNode(), matchedPredSuccInfo.BottomRef(0).m_stmt->GetRootNode())) - { - matchedPredSuccInfo.Emplace(succBlock, firstStmt); - } - else - { - return false; - } - } + return true; + }; - Statement* firstStmt = matchedPredSuccInfo.TopRef(0).m_stmt; - JITDUMP("All succs of " FMT_BB " start with the same tree\n", block->bbNum); - DISPSTMT(firstStmt); + Statement* nextFirstStmt; + Statement* destFirstStmt; - JITDUMP("Checking if we can move it into the predecessor...\n"); + if (!getSuccCandidate(block->bbNext, &nextFirstStmt) || !getSuccCandidate(block->bbJumpDest, &destFirstStmt)) + { + return false; + } - if (!fgCanMoveFirstStatementIntoPred(early, firstStmt, block)) + if (!GenTree::Compare(nextFirstStmt->GetRootNode(), destFirstStmt->GetRootNode())) { return false; } - JITDUMP("Moving statement\n"); + JITDUMP("Both succs of " FMT_BB " start with the same tree\n", block->bbNum); + DISPSTMT(nextFirstStmt); - for (int i = 0; i < matchedPredSuccInfo.Height(); i++) + if (gtTreeContainsTailCall(nextFirstStmt->GetRootNode()) || gtTreeContainsTailCall(destFirstStmt->GetRootNode())) { - PredSuccInfo& info = matchedPredSuccInfo.TopRef(i); - Statement* const stmt = info.m_stmt; - BasicBlock* const succBlock = info.m_block; + JITDUMP("But one is a tailcall\n"); + return false; + } - fgUnlinkStmt(succBlock, stmt); + JITDUMP("Checking if we can move it into the predecessor...\n"); - // Add one of the matching stmts to block, and - // update its flags. - // - if (i == 0) - { - fgInsertStmtNearEnd(block, stmt); - block->bbFlags |= succBlock->bbFlags & BBF_COPY_PROPAGATE; - } + if (!fgCanMoveFirstStatementIntoPred(early, nextFirstStmt, block)) + { + return false; } + JITDUMP("We can; moving statement\n"); + + fgUnlinkStmt(block->bbNext, nextFirstStmt); + fgInsertStmtNearEnd(block, nextFirstStmt); + fgUnlinkStmt(block->bbJumpDest, destFirstStmt); + block->bbFlags |= block->bbNext->bbFlags & BBF_COPY_PROPAGATE; + return true; } @@ -7334,18 +7158,17 @@ bool Compiler::fgTryOneHeadMerge(BasicBlock* block, ArrayStack& ma // // Parameters: // block - The block whose successors are to be considered -// matchedPredSuccInfo - Storage data structure // early - Whether this is being checked with early IR invariants // (where we do not have valid address exposure/GTF_GLOB_REF). // // Returns: // True if any merge succeeded. // -bool Compiler::fgHeadMerge(BasicBlock* block, ArrayStack& matchedPredSuccInfo, bool early) +bool Compiler::fgHeadMerge(BasicBlock* block, bool early) { bool madeChanges = false; int numOpts = 0; - while (fgTryOneHeadMerge(block, matchedPredSuccInfo, early)) + while (fgTryOneHeadMerge(block, early)) { madeChanges = true; numOpts++; @@ -7358,3 +7181,186 @@ bool Compiler::fgHeadMerge(BasicBlock* block, ArrayStack& matchedP return madeChanges; } + +//------------------------------------------------------------------------ +// gtTreeContainsTailCall: Check if a tree contains any tail call or tail call +// candidate. +// +// Parameters: +// tree - The tree +// +// Remarks: +// While tail calls are generally expected to be top level nodes we do allow +// some other shapes of calls to be tail calls, including some cascading +// trivial assignments and casts. This function does a tree walk to check if +// any sub tree is a tail call. +// +bool Compiler::gtTreeContainsTailCall(GenTree* tree) +{ + struct HasTailCallCandidateVisitor : GenTreeVisitor + { + enum + { + DoPreOrder = true + }; + + HasTailCallCandidateVisitor(Compiler* comp) : GenTreeVisitor(comp) + { + } + + fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) + { + GenTree* node = *use; + if ((node->gtFlags & GTF_CALL) == 0) + { + return WALK_SKIP_SUBTREES; + } + + if (node->IsCall() && (node->AsCall()->CanTailCall() || node->AsCall()->IsTailCall())) + { + return WALK_ABORT; + } + + return WALK_CONTINUE; + } + }; + + HasTailCallCandidateVisitor visitor(this); + return visitor.WalkTree(&tree, nullptr) == WALK_ABORT; +} + +//------------------------------------------------------------------------ +// fgCanMoveFirstStatementIntoPred: Check if the first statement of a block can +// be moved into its predecessor. +// +// Parameters: +// early - Whether this is being checked with early IR invariants (where +// we do not have valid address exposure/GTF_GLOB_REF). +// firstStmt - The statement to move +// pred - The predecessor block +// +// Remarks: +// Unlike tail merging, for head merging we have to either spill the +// predecessor's terminator node, or reorder it with the head statement. +// Here we choose to reorder. +// +bool Compiler::fgCanMoveFirstStatementIntoPred(bool early, Statement* firstStmt, BasicBlock* pred) +{ + if (!pred->HasTerminator()) + { + return true; + } + + GenTree* tree1 = pred->lastStmt()->GetRootNode(); + GenTree* tree2 = firstStmt->GetRootNode(); + + GenTreeFlags tree1Flags = tree1->gtFlags; + GenTreeFlags tree2Flags = tree2->gtFlags; + + if (early) + { + tree1Flags |= gtHasLocalsWithAddrOp(tree1) ? GTF_GLOB_REF : GTF_EMPTY; + tree2Flags |= gtHasLocalsWithAddrOp(tree2) ? GTF_GLOB_REF : GTF_EMPTY; + } + + // We do not support embedded statements in the terminator node. + if ((tree1Flags & GTF_ASG) != 0) + { + JITDUMP(" no; terminator contains embedded store\n"); + return false; + } + if ((tree2Flags & GTF_ASG) != 0) + { + // Handle common case where the second statement is a top-level store. + if (!tree2->OperIsLocalStore()) + { + JITDUMP(" cannot reorder with GTF_ASG without top-level store"); + return false; + } + + GenTreeLclVarCommon* lcl = tree2->AsLclVarCommon(); + if ((lcl->Data()->gtFlags & GTF_ASG) != 0) + { + JITDUMP(" cannot reorder with embedded store"); + return false; + } + + LclVarDsc* dsc = lvaGetDesc(tree2->AsLclVarCommon()); + if ((tree1Flags & GTF_ALL_EFFECT) != 0) + { + if (early ? dsc->lvHasLdAddrOp : dsc->IsAddressExposed()) + { + JITDUMP(" cannot reorder store to exposed local with any side effect\n"); + return false; + } + + if (((tree1Flags & (GTF_CALL | GTF_EXCEPT)) != 0) && pred->HasPotentialEHSuccs(this)) + { + JITDUMP(" cannot reorder store with exception throwing tree and potential EH successor\n"); + return false; + } + } + + if (gtHasRef(tree1, lcl->GetLclNum())) + { + JITDUMP(" cannot reorder with interfering use\n"); + return false; + } + + if (dsc->lvIsStructField && gtHasRef(tree1, dsc->lvParentLcl)) + { + JITDUMP(" cannot reorder with interfering use of parent struct local\n"); + return false; + } + + if (dsc->lvPromoted) + { + for (int i = 0; i < dsc->lvFieldCnt; i++) + { + if (gtHasRef(tree1, dsc->lvFieldLclStart + i)) + { + JITDUMP(" cannot reorder with interfering use of struct field\n"); + return false; + } + } + } + + // We've validated that the store does not interfere. Get rid of the + // flag for the future checks. + tree2Flags &= ~GTF_ASG; + } + + if (((tree1Flags & GTF_CALL) != 0) && ((tree2Flags & GTF_ALL_EFFECT) != 0)) + { + JITDUMP(" cannot reorder call with any side effect\n"); + return false; + } + if (((tree1Flags & GTF_GLOB_REF) != 0) && ((tree2Flags & GTF_PERSISTENT_SIDE_EFFECTS) != 0)) + { + JITDUMP(" cannot reorder global reference with persistent side effects\n"); + return false; + } + if ((tree1Flags & GTF_ORDER_SIDEEFF) != 0) + { + if ((tree2Flags & (GTF_GLOB_REF | GTF_ORDER_SIDEEFF)) != 0) + { + JITDUMP(" cannot reorder ordering side effect\n"); + return false; + } + } + if ((tree2Flags & GTF_ORDER_SIDEEFF) != 0) + { + if ((tree1Flags & (GTF_GLOB_REF | GTF_ORDER_SIDEEFF)) != 0) + { + JITDUMP(" cannot reorder ordering side effect\n"); + return false; + } + } + if (((tree1Flags & GTF_EXCEPT) != 0) && ((tree2Flags & GTF_SIDE_EFFECT) != 0)) + { + JITDUMP(" cannot reorder exception with side effect\n"); + return false; + } + + return true; +} From 885682e6d3d6fc29c01b3eaee9848e7bf0550d4e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 17 Aug 2023 22:16:23 +0200 Subject: [PATCH 12/12] Nit --- src/coreclr/jit/fgopt.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 9e03366a14ff5..7d4c0f9b11ac4 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -6741,7 +6741,7 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) predInfo.Reset(); // Find the subset of preds that reach along non-critical edges - // and populate predSuccInfo. + // and populate predInfo. // for (BasicBlock* const predBlock : block->PredBlocks()) {