diff --git a/src/coreclr/src/jit/codegen.h b/src/coreclr/src/jit/codegen.h index 7ebedb57fef1f7..94201def12d978 100644 --- a/src/coreclr/src/jit/codegen.h +++ b/src/coreclr/src/jit/codegen.h @@ -1535,9 +1535,10 @@ class CodeGenPhase final : public Phase } protected: - virtual void DoPhase() override + virtual PhaseStatus DoPhase() override { (codeGen->*action)(); + return PhaseStatus::MODIFIED_EVERYTHING; } private: diff --git a/src/coreclr/src/jit/compiler.cpp b/src/coreclr/src/jit/compiler.cpp index 0d4fc4a704dc20..e1b3f304c4721d 100644 --- a/src/coreclr/src/jit/compiler.cpp +++ b/src/coreclr/src/jit/compiler.cpp @@ -4195,19 +4195,7 @@ void Compiler::compCompile(void** methodCodePtr, ULONG* methodCodeSize, JitFlags // Import: convert the instrs in each basic block to a tree based intermediate representation // - auto importPhase = [this]() { - fgImport(); - - assert(!fgComputePredsDone); - if (fgCheapPredsValid) - { - // Remove cheap predecessors before inlining and fat call transformation; - // allowing the cheap predecessor lists to be inserted causes problems - // with splitting existing blocks. - fgRemovePreds(); - } - }; - DoPhase(this, PHASE_IMPORTATION, importPhase); + DoPhase(this, PHASE_IMPORTATION, &Compiler::fgImport); // Transform indirect calls that require control flow expansion. // @@ -4412,11 +4400,15 @@ void Compiler::compCompile(void** methodCodePtr, ULONG* methodCodeSize, JitFlags // Clone code in finallys to reduce overhead for non-exceptional paths // - auto cloneFinallyPhase = [this]() { - fgCloneFinally(); - fgUpdateFinallyTargetFlags(); - }; - DoPhase(this, PHASE_CLONE_FINALLY, cloneFinallyPhase); + DoPhase(this, PHASE_CLONE_FINALLY, &Compiler::fgCloneFinally); + +#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + + // Update finally target flags after EH optimizations + // + DoPhase(this, PHASE_UPDATE_FINALLY_FLAGS, &Compiler::fgUpdateFinallyTargetFlags); + +#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) #if DEBUG if (lvaEnregEHVars) diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 64b719f160f988..a4b8d7a0ebedfc 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -1130,6 +1130,13 @@ enum class PhaseChecks CHECK_ALL }; +// Specify compiler data that a phase might modify +enum class PhaseStatus : unsigned +{ + MODIFIED_NOTHING, + MODIFIED_EVERYTHING +}; + // The following enum provides a simple 1:1 mapping to CLR API's enum API_ICorJitInfo_Names { @@ -4359,31 +4366,35 @@ class Compiler void fgInit(); - void fgImport(); + PhaseStatus fgImport(); - void fgTransformIndirectCalls(); + PhaseStatus fgTransformIndirectCalls(); - void fgTransformPatchpoints(); + PhaseStatus fgTransformPatchpoints(); - void fgInline(); + PhaseStatus fgInline(); - void fgRemoveEmptyTry(); + PhaseStatus fgRemoveEmptyTry(); - void fgRemoveEmptyFinally(); + PhaseStatus fgRemoveEmptyFinally(); - void fgMergeFinallyChains(); + PhaseStatus fgMergeFinallyChains(); - void fgCloneFinally(); + PhaseStatus fgCloneFinally(); void fgCleanupContinuation(BasicBlock* continuation); - void fgUpdateFinallyTargetFlags(); +#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + + PhaseStatus fgUpdateFinallyTargetFlags(); void fgClearAllFinallyTargetBits(); void fgAddFinallyTargetFlags(); - void fgTailMergeThrows(); +#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + + PhaseStatus fgTailMergeThrows(); void fgTailMergeThrowsFallThroughHelper(BasicBlock* predBlock, BasicBlock* nonCanonicalBlock, BasicBlock* canonicalBlock, diff --git a/src/coreclr/src/jit/compphases.h b/src/coreclr/src/jit/compphases.h index cf0463c5f6e948..0cafdc1793e3ca 100644 --- a/src/coreclr/src/jit/compphases.h +++ b/src/coreclr/src/jit/compphases.h @@ -38,6 +38,7 @@ CompPhaseNameMacro(PHASE_EMPTY_TRY, "Remove empty try", CompPhaseNameMacro(PHASE_EMPTY_FINALLY, "Remove empty finally", "EMPTYFIN", false, -1, false) CompPhaseNameMacro(PHASE_MERGE_FINALLY_CHAINS, "Merge callfinally chains", "MRGCFCHN", false, -1, false) CompPhaseNameMacro(PHASE_CLONE_FINALLY, "Clone finally", "CLONEFIN", false, -1, false) +CompPhaseNameMacro(PHASE_UPDATE_FINALLY_FLAGS, "Update finally target flags", "UPD-FTF", false, -1, false) CompPhaseNameMacro(PHASE_COMPUTE_PREDS, "Compute preds", "PREDS", false, -1, false) CompPhaseNameMacro(PHASE_EARLY_UPDATE_FLOW_GRAPH,"Update flow graph early pass", "UPD-FG-E", false, -1, false) CompPhaseNameMacro(PHASE_STR_ADRLCL, "Morph - Structs/AddrExp", "MOR-STRAL",false, -1, false) diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp index cc522f785268d7..e3fcc5c18c6b82 100644 --- a/src/coreclr/src/jit/flowgraph.cpp +++ b/src/coreclr/src/jit/flowgraph.cpp @@ -6886,12 +6886,13 @@ unsigned Compiler::fgGetNestingLevel(BasicBlock* block, unsigned* pFinallyNestin return curNesting; } -/***************************************************************************** - * - * Import the basic blocks of the procedure. - */ - -void Compiler::fgImport() +//------------------------------------------------------------------------ +// fgImport: read the IL forf the method and create jit IR +// +// Returns: +// phase status +// +PhaseStatus Compiler::fgImport() { impImport(); @@ -6936,6 +6937,16 @@ void Compiler::fgImport() { compInlineResult->SetImportedILSize(info.compILImportSize); } + + // Full preds are only used later on + assert(!fgComputePredsDone); + if (fgCheapPredsValid) + { + // Cheap predecessors are only used during importation + fgRemovePreds(); + } + + return PhaseStatus::MODIFIED_EVERYTHING; } /***************************************************************************** @@ -21922,30 +21933,48 @@ unsigned Compiler::fgCheckInlineDepthAndRecursion(InlineInfo* inlineInfo) return depth; } -/***************************************************************************** - * - * Inlining phase - */ - -void Compiler::fgInline() +//------------------------------------------------------------------------ +// fgInline - expand inline candidates +// +// Returns: +// phase status indicating if anything was modified +// +// Notes: +// Inline candidates are identified during importation and candidate calls +// must be top-level expressions. In input IR, the result of the call (if any) +// is consumed elsewhere by a GT_RET_EXPR node. +// +// For successful inlines, calls are replaced by a sequence of argument setup +// instructions, the inlined method body, and return value cleanup. Note +// Inlining may introduce new inline candidates. These are processed in a +// depth-first fashion, as the inliner walks the IR in statement order. +// +// After inline expansion in a statement, the statement tree +// is walked to locate GT_RET_EXPR nodes. These are replaced by either +// * the original call tree, if the inline failed +// * the return value tree from the inlinee, if the inline succeeded +// +// This replacement happens in preorder; on the postorder side of the same +// tree walk, we look for opportunties to devirtualize or optimize now that +// we know the context for the newly supplied return value tree. +// +// Inline arguments may be directly substituted into the body of the inlinee +// in some cases. See impInlineFetchArg. +// +PhaseStatus Compiler::fgInline() { if (!opts.OptEnabled(CLFLG_INLINING)) { - return; + return PhaseStatus::MODIFIED_NOTHING; } #ifdef DEBUG - if (verbose) - { - printf("*************** In fgInline()\n"); - } - fgPrintInlinedMethods = JitConfig.JitPrintInlinedMethods().contains(info.compMethodName, info.compClassName, &info.compMethodInfo->args); - #endif // DEBUG - BasicBlock* block = fgFirstBB; + BasicBlock* block = fgFirstBB; + bool madeChanges = false; noway_assert(block != nullptr); // Set the root inline context on all statements @@ -21999,6 +22028,9 @@ void Compiler::fgInline() fgMorphCallInline(call, &inlineResult); + // If there's a candidate to process, we will make changes + madeChanges = true; + // fgMorphCallInline may have updated the // statement expression to a GT_NOP if the // call returned a value, regardless of @@ -22036,6 +22068,7 @@ void Compiler::fgInline() if (expr->OperGet() == GT_COMMA && expr->AsOp()->gtOp1->OperGet() == GT_CALL && expr->AsOp()->gtOp2->OperGet() == GT_NOP) { + madeChanges = true; stmt->SetRootNode(expr->AsOp()->gtOp1); } } @@ -22065,13 +22098,6 @@ void Compiler::fgInline() fgVerifyHandlerTab(); - if (verbose) - { - printf("*************** After fgInline()\n"); - fgDispBasicBlocks(true); - fgDispHandlerTab(); - } - if (verbose || fgPrintInlinedMethods) { JITDUMP("**************** Inline Tree"); @@ -22080,6 +22106,8 @@ void Compiler::fgInline() } #endif // DEBUG + + return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } #ifdef DEBUG @@ -23914,6 +23942,9 @@ void Compiler::fgLclFldAssign(unsigned lclNum) //------------------------------------------------------------------------ // fgRemoveEmptyFinally: Remove try/finallys where the finally is empty // +// Returns: +// PhaseStatus indicating what, if anything, was changed. +// // Notes: // Removes all try/finallys in the method with empty finallys. // These typically arise from inlining empty Dispose methods. @@ -23928,11 +23959,9 @@ void Compiler::fgLclFldAssign(unsigned lclNum) // empty (from say subsequent optimization). An SPMI run with // just the "detection" part of this phase run after optimization // found only one example where a new empty finally was detected. - -void Compiler::fgRemoveEmptyFinally() +// +PhaseStatus Compiler::fgRemoveEmptyFinally() { - JITDUMP("\n*************** In fgRemoveEmptyFinally()\n"); - #if defined(FEATURE_EH_FUNCLETS) // We need to do this transformation before funclets are created. assert(!fgFuncletsCreated); @@ -23944,19 +23973,19 @@ void Compiler::fgRemoveEmptyFinally() if (compHndBBtabCount == 0) { JITDUMP("No EH in this method, nothing to remove.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } if (opts.MinOpts()) { JITDUMP("Method compiled with minOpts, no removal.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } if (opts.compDbgCode) { JITDUMP("Method compiled with debug codegen, no removal.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } #ifdef DEBUG @@ -24151,16 +24180,18 @@ void Compiler::fgRemoveEmptyFinally() printf("\n"); } - fgVerifyHandlerTab(); - fgDebugCheckBBlist(false, false); - #endif // DEBUG } + + return (emptyCount > 0) ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } //------------------------------------------------------------------------ // fgRemoveEmptyTry: Optimize try/finallys where the try is empty // +// Returns: +// PhaseStatus indicating what, if anything, was changed. +// // Notes: // In runtimes where thread abort is not possible, `try {} finally {S}` // can be optimized to simply `S`. This method looks for such @@ -24173,8 +24204,8 @@ void Compiler::fgRemoveEmptyFinally() // this optimization, the code block `S` can lose special // within-finally status and so complete execution is no longer // guaranteed. - -void Compiler::fgRemoveEmptyTry() +// +PhaseStatus Compiler::fgRemoveEmptyTry() { JITDUMP("\n*************** In fgRemoveEmptyTry()\n"); @@ -24196,25 +24227,25 @@ void Compiler::fgRemoveEmptyTry() if (!enableRemoveEmptyTry) { JITDUMP("Empty try removal disabled.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } if (compHndBBtabCount == 0) { JITDUMP("No EH in this method, nothing to remove.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } if (opts.MinOpts()) { JITDUMP("Method compiled with minOpts, no removal.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } if (opts.compDbgCode) { JITDUMP("Method compiled with debug codegen, no removal.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } #ifdef DEBUG @@ -24470,26 +24501,18 @@ void Compiler::fgRemoveEmptyTry() { JITDUMP("fgRemoveEmptyTry() optimized %u empty-try try-finally clauses\n", emptyCount); fgOptimizedFinally = true; - -#ifdef DEBUG - if (verbose) - { - printf("\n*************** After fgRemoveEmptyTry()\n"); - fgDispBasicBlocks(); - fgDispHandlerTab(); - printf("\n"); - } - - fgVerifyHandlerTab(); - fgDebugCheckBBlist(false, false); - -#endif // DEBUG + return PhaseStatus::MODIFIED_EVERYTHING; } + + return PhaseStatus::MODIFIED_NOTHING; } //------------------------------------------------------------------------ // fgCloneFinally: Optimize normal exit path from a try/finally // +// Returns: +// PhaseStatus indicating what, if anything, was changed. +// // Notes: // Handles finallys that are not enclosed by or enclosing other // handler regions. @@ -24513,11 +24536,9 @@ void Compiler::fgRemoveEmptyTry() // BBF_CLONED_FINALLY_BEGIN and BBF_CLONED_FINALLY_END. However // these markers currently can get lost during subsequent // optimizations. - -void Compiler::fgCloneFinally() +// +PhaseStatus Compiler::fgCloneFinally() { - JITDUMP("\n*************** In fgCloneFinally()\n"); - #if defined(FEATURE_EH_FUNCLETS) // We need to do this transformation before funclets are created. assert(!fgFuncletsCreated); @@ -24536,31 +24557,30 @@ void Compiler::fgCloneFinally() if (!enableCloning) { JITDUMP("Finally cloning disabled.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } if (compHndBBtabCount == 0) { JITDUMP("No EH in this method, no cloning.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } if (opts.MinOpts()) { JITDUMP("Method compiled with minOpts, no cloning.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } if (opts.compDbgCode) { JITDUMP("Method compiled with debug codegen, no cloning.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } #ifdef DEBUG if (verbose) { - printf("\n*************** Before fgCloneFinally()\n"); fgDispBasicBlocks(); fgDispHandlerTab(); printf("\n"); @@ -25064,23 +25084,21 @@ void Compiler::fgCloneFinally() if (cloneCount > 0) { JITDUMP("fgCloneFinally() cloned %u finally handlers\n", cloneCount); - fgOptimizedFinally = true; #ifdef DEBUG if (verbose) { - printf("\n*************** After fgCloneFinally()\n"); fgDispBasicBlocks(); fgDispHandlerTab(); printf("\n"); } - fgVerifyHandlerTab(); - fgDebugCheckBBlist(false, false); fgDebugCheckTryFinallyExits(); #endif // DEBUG } + + return (cloneCount > 0 ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING); } #ifdef DEBUG @@ -25317,27 +25335,30 @@ void Compiler::fgCleanupContinuation(BasicBlock* continuation) #endif // !FEATURE_EH_FUNCLETS } -//------------------------------------------------------------------------ -// fgUpdateFinallyTargetFlags: recompute BBF_FINALLY_TARGET bits for all blocks -// after finally optimizations have run. - -void Compiler::fgUpdateFinallyTargetFlags() -{ #if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) - // Any fixup required? - if (!fgOptimizedFinally) +//------------------------------------------------------------------------ +// fgUpdateFinallyTargetFlags: recompute BBF_FINALLY_TARGET bits +// after EH optimizations +// +// Returns: +// phase status indicating if anything was modified +// +PhaseStatus Compiler::fgUpdateFinallyTargetFlags() +{ + // Any finally targetflag fixup required? + if (fgOptimizedFinally) { - JITDUMP("In fgUpdateFinallyTargetFlags - no finally opts, no fixup required\n"); - return; + JITDUMP("updating finally target flag bits\n"); + fgClearAllFinallyTargetBits(); + fgAddFinallyTargetFlags(); + return PhaseStatus::MODIFIED_EVERYTHING; + } + else + { + JITDUMP("no finally opts, no fixup required\n"); + return PhaseStatus::MODIFIED_NOTHING; } - - JITDUMP("In fgUpdateFinallyTargetFlags, updating finally target flag bits\n"); - - fgClearAllFinallyTargetBits(); - fgAddFinallyTargetFlags(); - -#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) } //------------------------------------------------------------------------ @@ -25346,8 +25367,6 @@ void Compiler::fgUpdateFinallyTargetFlags() // void Compiler::fgClearAllFinallyTargetBits() { -#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) - JITDUMP("*************** In fgClearAllFinallyTargetBits()\n"); // Note that we clear the flags even if there are no EH clauses (compHndBBtabCount == 0) @@ -25358,8 +25377,6 @@ void Compiler::fgClearAllFinallyTargetBits() { block->bbFlags &= ~BBF_FINALLY_TARGET; } - -#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) } //------------------------------------------------------------------------ @@ -25367,8 +25384,6 @@ void Compiler::fgClearAllFinallyTargetBits() // void Compiler::fgAddFinallyTargetFlags() { -#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) - JITDUMP("*************** In fgAddFinallyTargetFlags()\n"); if (compHndBBtabCount == 0) @@ -25393,12 +25408,16 @@ void Compiler::fgAddFinallyTargetFlags() } } } -#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) } +#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM) + //------------------------------------------------------------------------ // fgMergeFinallyChains: tail merge finally invocations // +// Returns: +// PhaseStatus indicating what, if anything, was changed. +// // Notes: // // Looks for common suffixes in chains of finally invocations @@ -25406,10 +25425,8 @@ void Compiler::fgAddFinallyTargetFlags() // try-finallys where there are multiple exit points in the try // that have the same target. -void Compiler::fgMergeFinallyChains() +PhaseStatus Compiler::fgMergeFinallyChains() { - JITDUMP("\n*************** In fgMergeFinallyChains()\n"); - #if defined(FEATURE_EH_FUNCLETS) // We need to do this transformation before funclets are created. assert(!fgFuncletsCreated); @@ -25421,19 +25438,19 @@ void Compiler::fgMergeFinallyChains() if (compHndBBtabCount == 0) { JITDUMP("No EH in this method, nothing to merge.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } if (opts.MinOpts()) { JITDUMP("Method compiled with minOpts, no merging.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } if (opts.compDbgCode) { JITDUMP("Method compiled with debug codegen, no merging.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } bool enableMergeFinallyChains = true; @@ -25458,13 +25475,12 @@ void Compiler::fgMergeFinallyChains() if (!enableMergeFinallyChains) { JITDUMP("fgMergeFinallyChains disabled\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } #ifdef DEBUG if (verbose) { - printf("\n*************** Before fgMergeFinallyChains()\n"); fgDispBasicBlocks(); fgDispHandlerTab(); printf("\n"); @@ -25488,7 +25504,7 @@ void Compiler::fgMergeFinallyChains() if (!hasFinally) { JITDUMP("Method does not have any try-finallys; no merging.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } // Process finallys from outside in, merging as we go. This gives @@ -25611,6 +25627,8 @@ void Compiler::fgMergeFinallyChains() "likely the non-canonical callfinallys were unreachable\n"); } } + + return didMerge ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } //------------------------------------------------------------------------ @@ -25768,6 +25786,9 @@ bool Compiler::fgUseThrowHelperBlocks() //------------------------------------------------------------------------ // fgTailMergeThrows: Tail merge throw blocks and blocks with no return calls. // +// Returns: +// PhaseStatus indicating what, if anything, was changed. +// // Notes: // Scans the flow graph for throw blocks and blocks with no return calls // that can be merged, and opportunistically merges them. @@ -25806,7 +25827,7 @@ bool Compiler::fgUseThrowHelperBlocks() // * CALL void ThrowHelper.ThrowArgumentOutOfRangeException // \--* CNS_INT int 33 // -void Compiler::fgTailMergeThrows() +PhaseStatus Compiler::fgTailMergeThrows() { noway_assert(opts.OptimizationEnabled()); @@ -25816,7 +25837,7 @@ void Compiler::fgTailMergeThrows() if (optNoReturnCallCount < 2) { JITDUMP("Method does not have multiple noreturn calls.\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } // This transformation requires block pred lists to be built @@ -25939,7 +25960,7 @@ void Compiler::fgTailMergeThrows() if (numCandidates == 0) { JITDUMP("\n*************** no throws can be tail merged, sorry\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } JITDUMP("\n*** found %d merge candidates, rewriting flow\n\n", numCandidates); @@ -26012,6 +26033,11 @@ void Compiler::fgTailMergeThrows() } } + if (updateCount == 0) + { + return PhaseStatus::MODIFIED_NOTHING; + } + // 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. @@ -26019,21 +26045,9 @@ void Compiler::fgTailMergeThrows() // 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. - if (updateCount > 0) - { - assert(fgModified); - fgModified = false; - } - -#if DEBUG - if (verbose) - { - printf("\n*************** After fgTailMergeThrows(%d updates)\n", updateCount); - fgDispBasicBlocks(); - fgVerifyHandlerTab(); - fgDebugCheckBBlist(); - } -#endif // DEBUG + assert(fgModified); + fgModified = false; + return PhaseStatus::MODIFIED_EVERYTHING; } //------------------------------------------------------------------------ diff --git a/src/coreclr/src/jit/indirectcalltransformer.cpp b/src/coreclr/src/jit/indirectcalltransformer.cpp index 6aadc37683a963..219e12902642d6 100644 --- a/src/coreclr/src/jit/indirectcalltransformer.cpp +++ b/src/coreclr/src/jit/indirectcalltransformer.cpp @@ -1018,23 +1018,24 @@ void Compiler::CheckNoTransformableIndirectCallsRemain() // These transformations happen post-import because they may introduce // control flow. // -void Compiler::fgTransformIndirectCalls() +// Returns: +// phase status indicating if changes were made +// +PhaseStatus Compiler::fgTransformIndirectCalls() { - JITDUMP("\n*************** in fgTransformIndirectCalls(%s)\n", compIsForInlining() ? "inlinee" : "root"); - + int count = 0; if (doesMethodHaveFatPointer() || doesMethodHaveGuardedDevirtualization() || doesMethodHaveExpRuntimeLookup()) { IndirectCallTransformer indirectCallTransformer(this); - int count = indirectCallTransformer.Run(); + count = indirectCallTransformer.Run(); if (count > 0) { - JITDUMP("\n*************** After fgTransformIndirectCalls() [%d calls transformed]\n", count); - INDEBUG(if (verbose) { fgDispBasicBlocks(true); }); + JITDUMP("\n -- %d calls transformed\n", count); } else { - JITDUMP(" -- no transforms done (?)\n"); + JITDUMP("\n -- no transforms done (?)\n"); } clearMethodHasFatPointer(); @@ -1043,8 +1044,10 @@ void Compiler::fgTransformIndirectCalls() } else { - JITDUMP(" -- no candidates to transform\n"); + JITDUMP("\n -- no candidates to transform\n"); } INDEBUG(CheckNoTransformableIndirectCallsRemain();); + + return (count == 0) ? PhaseStatus::MODIFIED_NOTHING : PhaseStatus::MODIFIED_EVERYTHING; } diff --git a/src/coreclr/src/jit/lower.cpp b/src/coreclr/src/jit/lower.cpp index f4d5b28edb7a82..3e79393cce1a20 100644 --- a/src/coreclr/src/jit/lower.cpp +++ b/src/coreclr/src/jit/lower.cpp @@ -5227,7 +5227,7 @@ GenTree* Lowering::LowerArrElem(GenTree* node) return nextToLower; } -void Lowering::DoPhase() +PhaseStatus Lowering::DoPhase() { // If we have any PInvoke calls, insert the one-time prolog code. We'll inserted the epilog code in the // appropriate spots later. NOTE: there is a minor optimization opportunity here, as we still create p/invoke @@ -5291,6 +5291,8 @@ void Lowering::DoPhase() // impact of any dead code removal. Note this may leave us with // tracked vars that have zero refs. comp->lvaComputeRefCounts(isRecompute, setSlotNumbers); + + return PhaseStatus::MODIFIED_EVERYTHING; } #ifdef DEBUG diff --git a/src/coreclr/src/jit/lower.h b/src/coreclr/src/jit/lower.h index 9cc09e5d377eae..838b86e74f4c49 100644 --- a/src/coreclr/src/jit/lower.h +++ b/src/coreclr/src/jit/lower.h @@ -28,7 +28,7 @@ class Lowering final : public Phase m_lsra = (LinearScan*)lsra; assert(m_lsra); } - virtual void DoPhase() override; + virtual PhaseStatus DoPhase() override; // This variant of LowerRange is called from outside of the main Lowering pass, // so it creates its own instance of Lowering to do so. diff --git a/src/coreclr/src/jit/objectalloc.cpp b/src/coreclr/src/jit/objectalloc.cpp index 79a2685d601f9f..5c31ca33ecef61 100644 --- a/src/coreclr/src/jit/objectalloc.cpp +++ b/src/coreclr/src/jit/objectalloc.cpp @@ -23,16 +23,18 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX // morph each GT_ALLOCOBJ node either into an allocation helper // call or stack allocation. // +// Returns: +// PhaseStatus indicating, what, if anything, was modified +// // Notes: // Runs only if Compiler::optMethodFlags has flag OMF_HAS_NEWOBJ set. - -void ObjectAllocator::DoPhase() +// +PhaseStatus ObjectAllocator::DoPhase() { - JITDUMP("\n*** ObjectAllocationPhase: "); if ((comp->optMethodFlags & OMF_HAS_NEWOBJ) == 0) { JITDUMP("no newobjs in this method; punting\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } if (IsObjectStackAllocationEnabled()) @@ -51,6 +53,11 @@ void ObjectAllocator::DoPhase() { ComputeStackObjectPointers(&m_bitVecTraits); RewriteUses(); + return PhaseStatus::MODIFIED_EVERYTHING; + } + else + { + return PhaseStatus::MODIFIED_NOTHING; } } diff --git a/src/coreclr/src/jit/objectalloc.h b/src/coreclr/src/jit/objectalloc.h index fa4aff552c231c..2d00c9db2a9cc0 100644 --- a/src/coreclr/src/jit/objectalloc.h +++ b/src/coreclr/src/jit/objectalloc.h @@ -45,7 +45,7 @@ class ObjectAllocator final : public Phase void EnableObjectStackAllocation(); protected: - virtual void DoPhase() override; + virtual PhaseStatus DoPhase() override; private: bool CanAllocateLclVarOnStack(unsigned int lclNum, CORINFO_CLASS_HANDLE clsHnd); diff --git a/src/coreclr/src/jit/patchpoint.cpp b/src/coreclr/src/jit/patchpoint.cpp index 8527bd937188c5..7116e780f4cd9b 100644 --- a/src/coreclr/src/jit/patchpoint.cpp +++ b/src/coreclr/src/jit/patchpoint.cpp @@ -195,14 +195,15 @@ class PatchpointTransformer // Patchpoints are placed in the JIT IR during importation, and get expanded // here into normal JIT IR. // -void Compiler::fgTransformPatchpoints() +// Returns: +// phase status indicating if changes were made +// +PhaseStatus Compiler::fgTransformPatchpoints() { - JITDUMP("\n*************** in fgTransformPatchpoints\n"); - if (!doesMethodHavePatchpoints()) { - JITDUMP(" -- no patchpoints to transform\n"); - return; + JITDUMP("\n -- no patchpoints to transform\n"); + return PhaseStatus::MODIFIED_NOTHING; } // We should only be adding patchpoints at Tier0, so should not be in an inlinee @@ -217,8 +218,8 @@ void Compiler::fgTransformPatchpoints() // optimizing the method (ala QJFL=0). if (compLocallocUsed) { - JITDUMP(" -- unable to handle methods with localloc\n"); - return; + JITDUMP("\n -- unable to handle methods with localloc\n"); + return PhaseStatus::MODIFIED_NOTHING; } // We currently can't do OSR in synchronized methods. We need to alter @@ -227,18 +228,18 @@ void Compiler::fgTransformPatchpoints() // obtained flag to true (or reuse the original method slot value). if ((info.compFlags & CORINFO_FLG_SYNCH) != 0) { - JITDUMP(" -- unable to handle synchronized methods\n"); - return; + JITDUMP("\n -- unable to handle synchronized methods\n"); + return PhaseStatus::MODIFIED_NOTHING; } if (opts.IsReversePInvoke()) { JITDUMP(" -- unable to handle Reverse P/Invoke\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } PatchpointTransformer ppTransformer(this); int count = ppTransformer.Run(); - JITDUMP("\n*************** After fgTransformPatchpoints() [%d patchpoints transformed]\n", count); - INDEBUG(if (verbose) { fgDispBasicBlocks(true); }); + JITDUMP("\n -- %d patchpoints transformed\n", count); + return (count == 0) ? PhaseStatus::MODIFIED_NOTHING : PhaseStatus::MODIFIED_EVERYTHING; } diff --git a/src/coreclr/src/jit/phase.cpp b/src/coreclr/src/jit/phase.cpp index ff67699cf196dc..fe1c9c008da976 100644 --- a/src/coreclr/src/jit/phase.cpp +++ b/src/coreclr/src/jit/phase.cpp @@ -9,14 +9,59 @@ #include "phase.h" +//------------------------------------------------------------------------ +// Observations ctor: snapshot key compiler variables before running a phase +// +// Arguments: +// compiler - current compiler instance +// +Phase::Observations::Observations(Compiler* compiler) +{ +#ifdef DEBUG + m_compiler = compiler->impInlineRoot(); + m_fgBBcount = m_compiler->fgBBcount; + m_fgBBNumMax = m_compiler->fgBBNumMax; + m_compHndBBtabCount = m_compiler->compHndBBtabCount; + m_lvaCount = m_compiler->lvaCount; + m_compGenTreeID = m_compiler->compGenTreeID; + m_compStatementID = m_compiler->compStatementID; + m_compBasicBlockID = m_compiler->compBasicBlockID; +#endif +} + +//------------------------------------------------------------------------ +// Observations Check: verify key compiler variables are unchanged +// if phase claims it made no modifications +// +// Arguments: +// status - status from the just-completed phase +// +void Phase::Observations::Check(PhaseStatus status) +{ +#ifdef DEBUG + if (status == PhaseStatus::MODIFIED_NOTHING) + { + assert(m_fgBBcount == m_compiler->fgBBcount); + assert(m_fgBBNumMax == m_compiler->fgBBNumMax); + assert(m_compHndBBtabCount == m_compiler->compHndBBtabCount); + assert(m_lvaCount == m_compiler->lvaCount); + assert(m_compGenTreeID == m_compiler->compGenTreeID); + assert(m_compStatementID == m_compiler->compStatementID); + assert(m_compBasicBlockID == m_compiler->compBasicBlockID); + } +#endif +} + //------------------------------------------------------------------------ // Run: execute a phase and any before and after actions // void Phase::Run() { + Observations observations(comp); PrePhase(); - DoPhase(); - PostPhase(); + PhaseStatus status = DoPhase(); + PostPhase(status); + observations.Check(status); } //------------------------------------------------------------------------ @@ -40,9 +85,8 @@ void Phase::PrePhase() // // Currently the list is just the set of phases that have custom // derivations from the Phase class. - static Phases s_whitelist[] = {PHASE_ALLOCATE_OBJECTS, PHASE_BUILD_SSA, PHASE_RATIONALIZE, PHASE_LOWERING, - PHASE_STACK_LEVEL_SETTER}; - bool doPrePhase = false; + static Phases s_whitelist[] = {PHASE_BUILD_SSA, PHASE_RATIONALIZE, PHASE_LOWERING, PHASE_STACK_LEVEL_SETTER}; + bool doPrePhase = false; for (int i = 0; i < sizeof(s_whitelist) / sizeof(Phases); i++) { @@ -88,10 +132,18 @@ void Phase::PrePhase() //------------------------------------------------------------------------ // PostPhase: perform dumps and checks after a phase executes // -void Phase::PostPhase() +// Arguments: +// status - status from the DoPhase call for this phase +// +void Phase::PostPhase(PhaseStatus status) { #ifdef DEBUG + // Don't dump or check post phase unless the phase made changes. + const bool madeChanges = (status != PhaseStatus::MODIFIED_NOTHING); + const char* const statusMessage = madeChanges ? "" : " [no changes]"; + bool doPostPhase = false; + // To help in the incremental conversion of jit activity to phases // without greatly increasing dump size or checked jit time, we // currently whitelist the phases that do post-phase checks and @@ -99,21 +151,28 @@ void Phase::PostPhase() // the various methods in the phase. // // As we remove the explicit checks and dumps from each phase, we - // will add to thist list; once all phases are updated, we can + // will add to this list; once all phases are updated, we can // remove the list entirely. // - // Currently the list is just the set of phases that have custom - // derivations from the Phase class. - static Phases s_whitelist[] = {PHASE_ALLOCATE_OBJECTS, PHASE_BUILD_SSA, PHASE_RATIONALIZE, PHASE_LOWERING, - PHASE_STACK_LEVEL_SETTER}; - bool doPostPhase = false; + // This list includes custom derivations from the Phase class as + // well as the new-style phases that have been updated to return + // PhaseStatus from their DoPhase methods. + // + static Phases s_whitelist[] = + {PHASE_IMPORTATION, PHASE_INDXCALL, PHASE_MORPH_INLINE, PHASE_ALLOCATE_OBJECTS, + PHASE_EMPTY_TRY, PHASE_EMPTY_FINALLY, PHASE_MERGE_FINALLY_CHAINS, PHASE_CLONE_FINALLY, + PHASE_MERGE_THROWS, PHASE_BUILD_SSA, PHASE_RATIONALIZE, PHASE_LOWERING, + PHASE_STACK_LEVEL_SETTER}; - for (int i = 0; i < sizeof(s_whitelist) / sizeof(Phases); i++) + if (madeChanges) { - if (m_phase == s_whitelist[i]) + for (int i = 0; i < sizeof(s_whitelist) / sizeof(Phases); i++) { - doPostPhase = true; - break; + if (m_phase == s_whitelist[i]) + { + doPostPhase = true; + break; + } } } @@ -121,12 +180,12 @@ void Phase::PostPhase() { if (comp->compIsForInlining()) { - printf("\n*************** Inline @[%06u] Finishing PHASE %s\n", - Compiler::dspTreeID(comp->impInlineInfo->iciCall), m_name); + printf("\n*************** Inline @[%06u] Finishing PHASE %s%s\n", + Compiler::dspTreeID(comp->impInlineInfo->iciCall), m_name, statusMessage); } else { - printf("\n*************** Finishing PHASE %s\n", m_name); + printf("\n*************** Finishing PHASE %s%s\n", m_name, statusMessage); } if (doPostPhase) @@ -147,6 +206,7 @@ void Phase::PostPhase() comp->fgDebugCheckBBlist(); comp->fgDebugCheckLinks(); comp->fgDebugCheckNodesUniqueness(); + comp->fgVerifyHandlerTab(); } } diff --git a/src/coreclr/src/jit/phase.h b/src/coreclr/src/jit/phase.h index 29a3399a5185d1..8f4c0f2de5ad9e 100644 --- a/src/coreclr/src/jit/phase.h +++ b/src/coreclr/src/jit/phase.h @@ -10,6 +10,27 @@ // class Phase { + // Observations made before a phase runs that should still + // be true afterwards,if the phase status is MODIFIED_NOTHING. + class Observations + { + public: + Observations(Compiler* compiler); + void Check(PhaseStatus status); + + private: +#ifdef DEBUG + Compiler* m_compiler; + unsigned m_fgBBcount; + unsigned m_fgBBNumMax; + unsigned m_compHndBBtabCount; + unsigned m_lvaCount; + unsigned m_compGenTreeID; + unsigned m_compStatementID; + unsigned m_compBasicBlockID; +#endif // DEBUG + }; + public: virtual void Run(); @@ -19,9 +40,9 @@ class Phase m_name = PhaseNames[_phase]; } - virtual void PrePhase(); - virtual void DoPhase() = 0; - virtual void PostPhase(); + virtual void PrePhase(); + virtual PhaseStatus DoPhase() = 0; + virtual void PostPhase(PhaseStatus status); Compiler* comp; const char* m_name; @@ -39,9 +60,10 @@ class ActionPhase final : public Phase } protected: - virtual void DoPhase() override + virtual PhaseStatus DoPhase() override { action(); + return PhaseStatus::MODIFIED_EVERYTHING; } private: @@ -68,9 +90,10 @@ class CompilerPhase final : public Phase } protected: - virtual void DoPhase() override + virtual PhaseStatus DoPhase() override { (comp->*action)(); + return PhaseStatus::MODIFIED_EVERYTHING; } private: @@ -85,4 +108,33 @@ inline void DoPhase(Compiler* _compiler, Phases _phase, void (Compiler::*_action phase.Run(); } +// A simple phase that just invokes a method on the compiler instance +// where the method being invoked returns a PhaseStatus +// +class CompilerPhaseWithStatus final : public Phase +{ +public: + CompilerPhaseWithStatus(Compiler* _compiler, Phases _phase, PhaseStatus (Compiler::*_action)()) + : Phase(_compiler, _phase), action(_action) + { + } + +protected: + virtual PhaseStatus DoPhase() override + { + return (comp->*action)(); + } + +private: + PhaseStatus (Compiler::*action)(); +}; + +// Wrapper for using CompilePhaseWithStatus +// +inline void DoPhase(Compiler* _compiler, Phases _phase, PhaseStatus (Compiler::*_action)()) +{ + CompilerPhaseWithStatus phase(_compiler, _phase, _action); + phase.Run(); +} + #endif /* End of _PHASE_H_ */ diff --git a/src/coreclr/src/jit/rationalize.cpp b/src/coreclr/src/jit/rationalize.cpp index 4518311f443bef..9f45bede3abbbf 100644 --- a/src/coreclr/src/jit/rationalize.cpp +++ b/src/coreclr/src/jit/rationalize.cpp @@ -817,7 +817,13 @@ Compiler::fgWalkResult Rationalizer::RewriteNode(GenTree** useEdge, Compiler::Ge return Compiler::WALK_CONTINUE; } -void Rationalizer::DoPhase() +//------------------------------------------------------------------------ +// DoPhase: Run the rationalize over the method IR. +// +// Returns: +// PhaseStatus indicating, what, if anything, was modified +// +PhaseStatus Rationalizer::DoPhase() { class RationalizeVisitor final : public GenTreeVisitor { @@ -911,4 +917,6 @@ void Rationalizer::DoPhase() } comp->compRationalIRForm = true; + + return PhaseStatus::MODIFIED_EVERYTHING; } diff --git a/src/coreclr/src/jit/rationalize.h b/src/coreclr/src/jit/rationalize.h index 2b35a498de2d4c..de836a292999f1 100644 --- a/src/coreclr/src/jit/rationalize.h +++ b/src/coreclr/src/jit/rationalize.h @@ -25,7 +25,7 @@ class Rationalizer final : public Phase #endif // DEBUG - virtual void DoPhase() override; + virtual PhaseStatus DoPhase() override; static void RewriteAssignmentIntoStoreLcl(GenTreeOp* assignment); diff --git a/src/coreclr/src/jit/stacklevelsetter.cpp b/src/coreclr/src/jit/stacklevelsetter.cpp index 15b77b22855fbd..0cf9dd34ade6fd 100644 --- a/src/coreclr/src/jit/stacklevelsetter.cpp +++ b/src/coreclr/src/jit/stacklevelsetter.cpp @@ -27,6 +27,9 @@ StackLevelSetter::StackLevelSetter(Compiler* compiler) //------------------------------------------------------------------------ // DoPhase: Calculate stack slots numbers for outgoing args. // +// Returns: +// PhaseStatus indicating what, if anything, was changed. +// // Notes: // For non-x86 platforms it calculates the max number of slots // that calls inside this method can push on the stack. @@ -37,7 +40,8 @@ StackLevelSetter::StackLevelSetter(Compiler* compiler) // For x86 it also sets throw-helper blocks incoming stack depth and set // framePointerRequired when it is necessary. These values are used to pop // pushed args when an exception occurs. -void StackLevelSetter::DoPhase() +// +PhaseStatus StackLevelSetter::DoPhase() { for (BasicBlock* block = comp->fgFirstBB; block != nullptr; block = block->bbNext) { @@ -55,6 +59,9 @@ void StackLevelSetter::DoPhase() comp->fgSetPtrArgCntMax(maxStackLevel); CheckArgCnt(); + + // Might want an "other" category for things like this... + return PhaseStatus::MODIFIED_NOTHING; } //------------------------------------------------------------------------ diff --git a/src/coreclr/src/jit/stacklevelsetter.h b/src/coreclr/src/jit/stacklevelsetter.h index 4ee9b99e645e14..b568980e67120b 100644 --- a/src/coreclr/src/jit/stacklevelsetter.h +++ b/src/coreclr/src/jit/stacklevelsetter.h @@ -12,7 +12,7 @@ class StackLevelSetter final : public Phase public: StackLevelSetter(Compiler* compiler); - virtual void DoPhase() override; + virtual PhaseStatus DoPhase() override; private: void ProcessBlock(BasicBlock* block);