diff --git a/src/coreclr/src/jit/compiler.cpp b/src/coreclr/src/jit/compiler.cpp index a757488847208e..dca1105f8138f0 100644 --- a/src/coreclr/src/jit/compiler.cpp +++ b/src/coreclr/src/jit/compiler.cpp @@ -4461,9 +4461,16 @@ void Compiler::compCompile(void** methodCodePtr, ULONG* methodCodeSize, JitFlags }; DoPhase(this, PHASE_COMPUTE_PREDS, computePredsPhase); - // Run an early flow graph simplification pass + // Now that we have pred lists, do some flow-related optimizations + // if (opts.OptimizationEnabled()) { + // Merge common throw blocks + // + DoPhase(this, PHASE_MERGE_THROWS, &Compiler::fgTailMergeThrows); + + // Run an early flow graph simplification pass + // auto earlyUpdateFlowGraphPhase = [this]() { const bool doTailDup = false; fgUpdateFlowGraph(doTailDup); @@ -4578,9 +4585,6 @@ void Compiler::compCompile(void** methodCodePtr, ULONG* methodCodeSize, JitFlags if (opts.OptimizationEnabled()) { - // Merge common throw blocks - // - DoPhase(this, PHASE_MERGE_THROWS, &Compiler::fgTailMergeThrows); // Optimize block order // DoPhase(this, PHASE_OPTIMIZE_LAYOUT, &Compiler::optOptimizeLayout); diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp index e8229286aaf39e..36f509fd5cd601 100644 --- a/src/coreclr/src/jit/flowgraph.cpp +++ b/src/coreclr/src/jit/flowgraph.cpp @@ -25823,6 +25823,10 @@ PhaseStatus Compiler::fgTailMergeThrows() JITDUMP("Method does not have multiple noreturn calls.\n"); return PhaseStatus::MODIFIED_NOTHING; } + else + { + JITDUMP("Scanning the %u candidates\n", optNoReturnCallCount); + } // This transformation requires block pred lists to be built // so that flow can be safely updated. @@ -25888,11 +25892,16 @@ PhaseStatus Compiler::fgTailMergeThrows() continue; } - // For throw helpers the block should have exactly one statement.... - // (this isn't guaranteed, but seems likely) - Statement* stmt = block->firstStmt(); + // We only look at the first statement for throw helper calls. + // Remainder of the block will be dead code. + // + // Throw helper calls could show up later in the block; we + // won't try merging those as we'd need to match up all the + // prior statements or split the block at this point, etc. + // + Statement* const stmt = block->firstStmt(); - if ((stmt == nullptr) || (stmt->GetNextStmt() != nullptr)) + if (stmt == nullptr) { continue; } @@ -25954,13 +25963,14 @@ PhaseStatus Compiler::fgTailMergeThrows() // We walk the map rather than the block list, to save a bit of time. BlockToBlockMap::KeyIterator iter(blockMap.Begin()); BlockToBlockMap::KeyIterator end(blockMap.End()); - int updateCount = 0; + unsigned updateCount = 0; for (; !iter.Equal(end); iter++) { BasicBlock* const nonCanonicalBlock = iter.Get(); BasicBlock* const canonicalBlock = iter.GetValue(); flowList* nextPredEdge = nullptr; + bool updated = false; // Walk pred list of the non canonical block, updating flow to target // the canonical block instead. @@ -25974,14 +25984,14 @@ PhaseStatus Compiler::fgTailMergeThrows() case BBJ_NONE: { fgTailMergeThrowsFallThroughHelper(predBlock, nonCanonicalBlock, canonicalBlock, predEdge); - updateCount++; + updated = true; } break; case BBJ_ALWAYS: { fgTailMergeThrowsJumpToHelper(predBlock, nonCanonicalBlock, canonicalBlock, predEdge); - updateCount++; + updated = true; } break; @@ -25997,7 +26007,7 @@ PhaseStatus Compiler::fgTailMergeThrows() { fgTailMergeThrowsJumpToHelper(predBlock, nonCanonicalBlock, canonicalBlock, predEdge); } - updateCount++; + updated = true; } break; @@ -26005,7 +26015,7 @@ PhaseStatus Compiler::fgTailMergeThrows() { JITDUMP("*** " FMT_BB " now branching to " FMT_BB "\n", predBlock->bbNum, canonicalBlock->bbNum); fgReplaceSwitchJumpTarget(predBlock, canonicalBlock, nonCanonicalBlock); - updateCount++; + updated = true; } break; @@ -26015,6 +26025,11 @@ PhaseStatus Compiler::fgTailMergeThrows() break; } } + + if (updated) + { + updateCount++; + } } if (updateCount == 0) @@ -26022,13 +26037,18 @@ PhaseStatus Compiler::fgTailMergeThrows() return PhaseStatus::MODIFIED_NOTHING; } + // TODO: Update the count of noreturn call sites -- this feeds a heuristic in morph + // to determine if these noreturn calls should be tail called. + // + // Updating the count does not lead to better results, so deferring for now. + // + JITDUMP("Made %u updates\n", updateCount); + assert(updateCount < optNoReturnCallCount); + // 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. // - // Note we could invoke a cleanup pass here, but optOptimizeFlow - // seems to be missing some safety checks and doesn't expect to - // see an already cleaned-up flow graph. assert(fgModified); fgModified = false; return PhaseStatus::MODIFIED_EVERYTHING; diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index f5b72fc566316a..98510552ca1bfe 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -1288,6 +1288,15 @@ bool GenTree::Compare(GenTree* op1, GenTree* op2, bool swapOK) return true; } break; + + case GT_CNS_STR: + if ((op1->AsStrCon()->gtSconCPX == op2->AsStrCon()->gtSconCPX) && + (op1->AsStrCon()->gtScpHnd == op2->AsStrCon()->gtScpHnd)) + { + return true; + } + break; + #if 0 // TODO-CQ: Enable this in the future case GT_CNS_LNG: diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index fac481935312ac..9e46d98c78116e 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -7101,6 +7101,10 @@ GenTree* Compiler::fgMorphPotentialTailCall(GenTreeCall* call) // Heuristic: regular calls to noreturn methods can sometimes be // merged, so if we have multiple such calls, we defer tail calling. + // + // TODO: re-examine this; now that we're merging before morph we + // don't need to worry about interfering with merges. + // if (call->IsNoReturn() && (optNoReturnCallCount > 1)) { failTailCall("Defer tail calling throw helper; anticipating merge");