From: Andy Ayers Date: Thu, 6 Feb 2020 02:34:15 +0000 (-0800) Subject: JIT: rework phase objects so we can use them more widely (#31808) X-Git-Tag: submit/tizen/20210909.063632~9970 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=5040afeb94855b630ba822a9d62162c3652fe392;p=platform%2Fupstream%2Fdotnet%2Fruntime.git JIT: rework phase objects so we can use them more widely (#31808) Next part of #2109. Add support for phases that are simply compiler methods or lambdas. These are not used yet -- the plan is to introduce new phases gradually, cleaning up redundant checking and dumping along the way. This will happen in subsequent changes. Remove a bit of now-redundant post-phase dumping and checking from lower. Add the active phase to the assertion text, so we have some context. --- diff --git a/src/coreclr/src/jit/CMakeLists.txt b/src/coreclr/src/jit/CMakeLists.txt index bc91b7b..b63e08a 100644 --- a/src/coreclr/src/jit/CMakeLists.txt +++ b/src/coreclr/src/jit/CMakeLists.txt @@ -67,6 +67,7 @@ set( JIT_SOURCES objectalloc.cpp optcse.cpp optimizer.cpp + phase.cpp rangecheck.cpp rationalize.cpp regalloc.cpp diff --git a/src/coreclr/src/jit/compiler.cpp b/src/coreclr/src/jit/compiler.cpp index 35d0c0d..f4bc154 100644 --- a/src/coreclr/src/jit/compiler.cpp +++ b/src/coreclr/src/jit/compiler.cpp @@ -1751,6 +1751,12 @@ void Compiler::compInit(ArenaAllocator* pAlloc, InlineInfo* inlineInfo) compInlineResult = nullptr; } + // Initialize this to the first phase to run. + mostRecentlyActivePhase = PHASE_PRE_IMPORT; + + // Initially, no phase checks are active. + activePhaseChecks = PhaseChecks::CHECK_NONE; + #ifdef FEATURE_TRACELOGGING // Make sure JIT telemetry is initialized as soon as allocations can be made // but no later than a point where noway_asserts can be thrown. @@ -1759,9 +1765,8 @@ void Compiler::compInit(ArenaAllocator* pAlloc, InlineInfo* inlineInfo) // Note: JIT telemetry could gather data when compiler is not fully initialized. // So you have to initialize the compiler variables you use for telemetry. assert((unsigned)PHASE_PRE_IMPORT == 0); - previousCompletedPhase = PHASE_PRE_IMPORT; - info.compILCodeSize = 0; - info.compMethodHnd = nullptr; + info.compILCodeSize = 0; + info.compMethodHnd = nullptr; compJitTelemetry.Initialize(this); #endif @@ -4215,6 +4220,37 @@ void Compiler::compFunctionTraceEnd(void* methodCodePtr, ULONG methodCodeSize, b } //------------------------------------------------------------------------ +// BeginPhase: begin execution of a phase +// +// Arguments: +// phase - the phase that is about to begin +// +void Compiler::BeginPhase(Phases phase) +{ + mostRecentlyActivePhase = phase; +} + +//------------------------------------------------------------------------ +// EndPhase: finish execution of a phase +// +// Arguments: +// phase - the phase that has just finished +// +void Compiler::EndPhase(Phases phase) +{ +#if defined(FEATURE_JIT_METHOD_PERF) + if (pCompJitTimer != nullptr) + { + pCompJitTimer->EndPhase(this, phase); + } +#endif +#if DUMP_FLOWGRAPHS + fgDumpFlowGraph(phase); +#endif // DUMP_FLOWGRAPHS + mostRecentlyActivePhase = phase; +} + +//------------------------------------------------------------------------ // compCompile: run phases needed for compilation // // Arguments: @@ -4564,7 +4600,10 @@ void Compiler::compCompile(void** methodCodePtr, ULONG* methodCodeSize, JitFlags #endif // DEBUG // End of the morphing phases + // + // We can now enable all phase checking EndPhase(PHASE_MORPH_END); + activePhaseChecks = PhaseChecks::CHECK_ALL; // GS security checks for unsafe buffers if (getNeedsGSSecurityCookie()) diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 098a170..33a1859 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -1094,6 +1094,14 @@ extern const char* PhaseNames[]; extern const char* PhaseEnums[]; extern const LPCWSTR PhaseShortNames[]; +// Specify which checks should be run after each phase +// +enum class PhaseChecks +{ + CHECK_NONE, + CHECK_ALL +}; + // The following enum provides a simple 1:1 mapping to CLR API's enum API_ICorJitInfo_Names { @@ -8979,7 +8987,8 @@ public: #endif // !TARGET_X86 - Phases previousCompletedPhase; // the most recently completed phase + Phases mostRecentlyActivePhase; // the most recently active phase + PhaseChecks activePhaseChecks; // the currently active phase checks //------------------------------------------------------------------------- // The following keeps track of how many bytes of local frame space we've @@ -9492,7 +9501,8 @@ private: static LPCWSTR JitTimeLogCsv(); // Retrieve the file name for CSV from ConfigDWORD. static LPCWSTR compJitTimeLogFilename; // If a log file for JIT time is desired, filename to write it to. #endif - inline void EndPhase(Phases phase); // Indicate the end of the given phase. + void BeginPhase(Phases phase); // Indicate the start of the given phase. + void EndPhase(Phases phase); // Indicate the end of the given phase. #if MEASURE_CLRAPI_CALLS // Thin wrappers that call into JitTimer (if present). diff --git a/src/coreclr/src/jit/compiler.hpp b/src/coreclr/src/jit/compiler.hpp index 429a583..d0de0f1 100644 --- a/src/coreclr/src/jit/compiler.hpp +++ b/src/coreclr/src/jit/compiler.hpp @@ -4075,20 +4075,6 @@ inline bool Compiler::lvaIsGCTracked(const LclVarDsc* varDsc) } } -inline void Compiler::EndPhase(Phases phase) -{ -#if defined(FEATURE_JIT_METHOD_PERF) - if (pCompJitTimer != nullptr) - { - pCompJitTimer->EndPhase(this, phase); - } -#endif -#if DUMP_FLOWGRAPHS - fgDumpFlowGraph(phase); -#endif // DUMP_FLOWGRAPHS - previousCompletedPhase = phase; -} - /*****************************************************************************/ #if MEASURE_CLRAPI_CALLS diff --git a/src/coreclr/src/jit/error.cpp b/src/coreclr/src/jit/error.cpp index dd020e5..90a2a76 100644 --- a/src/coreclr/src/jit/error.cpp +++ b/src/coreclr/src/jit/error.cpp @@ -272,10 +272,12 @@ extern "C" void __cdecl assertAbort(const char* why, const char* file, unsigned LogEnv* env = JitTls::GetLogEnv(); const int BUFF_SIZE = 8192; char* buff = (char*)alloca(BUFF_SIZE); + const char* phaseName = "unknown phase"; if (env->compiler) { - _snprintf_s(buff, BUFF_SIZE, _TRUNCATE, "Assertion failed '%s' in '%s' (IL size %d)\n", why, - env->compiler->info.compFullName, env->compiler->info.compILCodeSize); + phaseName = PhaseNames[env->compiler->mostRecentlyActivePhase]; + _snprintf_s(buff, BUFF_SIZE, _TRUNCATE, "Assertion failed '%s' in '%s' during '%s' (IL size %d)\n", why, + env->compiler->info.compFullName, phaseName, env->compiler->info.compILCodeSize); msg = buff; } printf(""); // null string means flush @@ -283,8 +285,8 @@ extern "C" void __cdecl assertAbort(const char* why, const char* file, unsigned #if FUNC_INFO_LOGGING if (Compiler::compJitFuncInfoFile != nullptr) { - fprintf(Compiler::compJitFuncInfoFile, "%s - Assertion failed (%s:%d - %s)\n", - (env == nullptr) ? "UNKNOWN" : env->compiler->info.compFullName, file, line, why); + fprintf(Compiler::compJitFuncInfoFile, "%s - Assertion failed (%s:%d - %s) during %s\n", + (env == nullptr) ? "UNKNOWN" : env->compiler->info.compFullName, file, line, why, phaseName); } #endif // FUNC_INFO_LOGGING diff --git a/src/coreclr/src/jit/jit.settings.targets b/src/coreclr/src/jit/jit.settings.targets index 9c567c0..666b409 100644 --- a/src/coreclr/src/jit/jit.settings.targets +++ b/src/coreclr/src/jit/jit.settings.targets @@ -93,6 +93,7 @@ + diff --git a/src/coreclr/src/jit/jittelemetry.cpp b/src/coreclr/src/jit/jittelemetry.cpp index 021fb45..3861789 100644 --- a/src/coreclr/src/jit/jittelemetry.cpp +++ b/src/coreclr/src/jit/jittelemetry.cpp @@ -273,7 +273,7 @@ void JitTelemetry::NotifyNowayAssert(const char* filename, unsigned line) { codeSize = comp->info.compILCodeSize; minOpts = comp->opts.IsMinOptsSet() ? comp->opts.MinOpts() : -1; - lastPhase = PhaseNames[comp->previousCompletedPhase]; + lastPhase = PhaseNames[comp->mostRecentlyActivePhase]; } CacheCurrentMethodInfo(); diff --git a/src/coreclr/src/jit/lower.cpp b/src/coreclr/src/jit/lower.cpp index bf47b52..a88f895 100644 --- a/src/coreclr/src/jit/lower.cpp +++ b/src/coreclr/src/jit/lower.cpp @@ -5326,19 +5326,6 @@ 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); - -#ifdef DEBUG - JITDUMP("Liveness pass finished after lowering, IR:\n"); - if (VERBOSE) - { - comp->fgDispBasicBlocks(true); - } - - for (BasicBlock* block = comp->fgFirstBB; block; block = block->bbNext) - { - assert(LIR::AsRange(block).CheckLIR(comp, true)); - } -#endif } #ifdef DEBUG diff --git a/src/coreclr/src/jit/lower.h b/src/coreclr/src/jit/lower.h index 1ad3a72..3fef773 100644 --- a/src/coreclr/src/jit/lower.h +++ b/src/coreclr/src/jit/lower.h @@ -19,11 +19,11 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX #include "lsra.h" #include "sideeffects.h" -class Lowering : public Phase +class Lowering final : public Phase { public: inline Lowering(Compiler* compiler, LinearScanInterface* lsra) - : Phase(compiler, "Lowering", PHASE_LOWERING), vtableCallTemp(BAD_VAR_NUM) + : Phase(compiler, PHASE_LOWERING), vtableCallTemp(BAD_VAR_NUM) { m_lsra = (LinearScan*)lsra; assert(m_lsra); diff --git a/src/coreclr/src/jit/objectalloc.h b/src/coreclr/src/jit/objectalloc.h index 76f5ec9..fa4aff5 100644 --- a/src/coreclr/src/jit/objectalloc.h +++ b/src/coreclr/src/jit/objectalloc.h @@ -76,15 +76,12 @@ private: //=============================================================================== inline ObjectAllocator::ObjectAllocator(Compiler* comp) - : Phase(comp, "Allocate Objects", PHASE_ALLOCATE_OBJECTS) + : Phase(comp, PHASE_ALLOCATE_OBJECTS) , m_IsObjectStackAllocationEnabled(false) , m_AnalysisDone(false) , m_bitVecTraits(comp->lvaCount, comp) , m_HeapLocalToStackLocalMap(comp->getAllocator()) { - // Disable checks since this phase runs before fgComputePreds phase. - // Checks are not expected to pass before fgComputePreds. - doChecks = false; m_EscapingPointers = BitVecOps::UninitVal(); m_PossiblyStackPointingPointers = BitVecOps::UninitVal(); m_DefinitelyStackPointingPointers = BitVecOps::UninitVal(); diff --git a/src/coreclr/src/jit/phase.cpp b/src/coreclr/src/jit/phase.cpp new file mode 100644 index 0000000..1d5b055 --- /dev/null +++ b/src/coreclr/src/jit/phase.cpp @@ -0,0 +1,93 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#include "jitpch.h" +#ifdef _MSC_VER +#pragma hdrstop +#endif + +#include "phase.h" + +//------------------------------------------------------------------------ +// Run: execute a phase and any before and after actions +// +void Phase::Run() +{ + PrePhase(); + DoPhase(); + PostPhase(); +} + +//------------------------------------------------------------------------ +// PrePhase: perform dumps and checks before a phase executes +// +void Phase::PrePhase() +{ + comp->BeginPhase(m_phase); + +#ifdef DEBUG + if (VERBOSE) + { + if (comp->compIsForInlining()) + { + printf("\n*************** Inline @[%06u] Starting PHASE %s\n", + Compiler::dspTreeID(comp->impInlineInfo->iciCall), m_name); + } + else + { + printf("\n*************** Starting PHASE %s\n", m_name); + } + + printf("Trees before %s\n", m_name); + comp->fgDispBasicBlocks(true); + } + + if ((comp->activePhaseChecks == PhaseChecks::CHECK_ALL) && (comp->expensiveDebugCheckLevel >= 2)) + { + // If everyone used the Phase class, this would duplicate the PostPhase() from the previous phase. + // But, not everyone does, so go ahead and do the check here, too. + comp->fgDebugCheckBBlist(); + comp->fgDebugCheckLinks(); + } +#endif // DEBUG +} + +//------------------------------------------------------------------------ +// PostPhase: perform dumps and checks after a phase executes +// +void Phase::PostPhase() +{ +#ifdef DEBUG + if (VERBOSE) + { + if (comp->compIsForInlining()) + { + printf("\n*************** Inline @[%06u] Finishing PHASE %s\n", + Compiler::dspTreeID(comp->impInlineInfo->iciCall), m_name); + } + else + { + printf("\n*************** Finishing PHASE %s\n", m_name); + } + + printf("Trees after %s\n", m_name); + comp->fgDispBasicBlocks(true); + } +#endif // DEBUG + +#if DUMP_FLOWGRAPHS + comp->fgDumpFlowGraph(m_phase); +#endif // DUMP_FLOWGRAPHS + +#ifdef DEBUG + if (comp->activePhaseChecks == PhaseChecks::CHECK_ALL) + { + comp->fgDebugCheckBBlist(); + comp->fgDebugCheckLinks(); + comp->fgDebugCheckNodesUniqueness(); + } +#endif // DEBUG + + comp->EndPhase(m_phase); +} diff --git a/src/coreclr/src/jit/phase.h b/src/coreclr/src/jit/phase.h index 2077f94..646a8a2 100644 --- a/src/coreclr/src/jit/phase.h +++ b/src/coreclr/src/jit/phase.h @@ -6,15 +6,17 @@ #ifndef _PHASE_H_ #define _PHASE_H_ +// A phase encapsulates a part of the compilation pipeline for a method. +// class Phase { public: virtual void Run(); protected: - Phase(Compiler* _comp, const char* _name, Phases _phase = PHASE_NUMBER_OF) - : comp(_comp), name(_name), phase(_phase), doChecks(true) + Phase(Compiler* _compiler, Phases _phase) : comp(_compiler), m_name(nullptr), m_phase(_phase) { + m_name = PhaseNames[_phase]; } virtual void PrePhase(); @@ -22,61 +24,66 @@ protected: virtual void PostPhase(); Compiler* comp; - const char* name; - Phases phase; - bool doChecks; + const char* m_name; + Phases m_phase; }; -inline void Phase::Run() +// A phase that accepts a lambda for the actions done by the phase. +// +// Would prefer to use std::function via here, but seemingly can't. +// +template +class ActionPhase final : public Phase { - PrePhase(); - DoPhase(); - PostPhase(); -} - -inline void Phase::PrePhase() -{ -#ifdef DEBUG - if (VERBOSE) +public: + ActionPhase(Compiler* _compiler, Phases _phase, A _action) : Phase(_compiler, _phase), action(_action) { - printf("*************** In %s\n", name); - printf("Trees before %s\n", name); - comp->fgDispBasicBlocks(true); } - if (doChecks && comp->expensiveDebugCheckLevel >= 2) +protected: + virtual void DoPhase() override { - // If everyone used the Phase class, this would duplicate the PostPhase() from the previous phase. - // But, not everyone does, so go ahead and do the check here, too. - comp->fgDebugCheckBBlist(); - comp->fgDebugCheckLinks(); + action(); } -#endif // DEBUG + +private: + A action; +}; + +// Wrapper for using ActionPhase +// +template +void DoPhase(Compiler* _compiler, Phases _phase, A _action) +{ + ActionPhase phase(_compiler, _phase, _action); + phase.Run(); } -inline void Phase::PostPhase() +// A simple phase that just invokes a method on the compiler instance +// +class CompilerPhase final : public Phase { -#ifdef DEBUG - if (VERBOSE) +public: + CompilerPhase(Compiler* _compiler, Phases _phase, void (Compiler::*_action)()) : Phase(_compiler, _phase) { - printf("*************** Exiting %s\n", name); - printf("Trees after %s\n", name); - comp->fgDispBasicBlocks(true); } -#endif // DEBUG - if (phase != PHASE_NUMBER_OF) +protected: + virtual void DoPhase() override { - comp->EndPhase(phase); + (comp->*action)(); } -#ifdef DEBUG - if (doChecks) - { - comp->fgDebugCheckBBlist(); - comp->fgDebugCheckLinks(); - } -#endif // DEBUG +private: + void (Compiler::*action)(); +}; + +// Wrapper for using CompilePhase +// +inline void DoPhase(Compiler* _compiler, Phases _phase, void (Compiler::*_action)()) +{ + CompilerPhase phase(_compiler, _phase, _action); + phase.Run(); } #endif /* End of _PHASE_H_ */ diff --git a/src/coreclr/src/jit/rationalize.h b/src/coreclr/src/jit/rationalize.h index d396ac8..2b35a49 100644 --- a/src/coreclr/src/jit/rationalize.h +++ b/src/coreclr/src/jit/rationalize.h @@ -5,7 +5,7 @@ //=============================================================================== #include "phase.h" -class Rationalizer : public Phase +class Rationalizer final : public Phase { private: BasicBlock* m_block; @@ -57,7 +57,7 @@ private: Compiler::fgWalkResult RewriteNode(GenTree** useEdge, Compiler::GenTreeStack& parents); }; -inline Rationalizer::Rationalizer(Compiler* _comp) : Phase(_comp, "IR Rationalize", PHASE_RATIONALIZE) +inline Rationalizer::Rationalizer(Compiler* _comp) : Phase(_comp, PHASE_RATIONALIZE) { #ifdef DEBUG comp->compNumStatementLinksTraversed = 0; diff --git a/src/coreclr/src/jit/stacklevelsetter.cpp b/src/coreclr/src/jit/stacklevelsetter.cpp index 4de2535..15b77b2 100644 --- a/src/coreclr/src/jit/stacklevelsetter.cpp +++ b/src/coreclr/src/jit/stacklevelsetter.cpp @@ -10,7 +10,7 @@ #include "stacklevelsetter.h" StackLevelSetter::StackLevelSetter(Compiler* compiler) - : Phase(compiler, "StackLevelSetter", PHASE_STACK_LEVEL_SETTER) + : Phase(compiler, PHASE_STACK_LEVEL_SETTER) , currentStackLevel(0) , maxStackLevel(0) , memAllocator(compiler->getAllocator(CMK_fgArgInfoPtrArr)) diff --git a/src/coreclr/src/jit/stacklevelsetter.h b/src/coreclr/src/jit/stacklevelsetter.h index 984cf54..4ee9b99 100644 --- a/src/coreclr/src/jit/stacklevelsetter.h +++ b/src/coreclr/src/jit/stacklevelsetter.h @@ -7,7 +7,7 @@ #include "compiler.h" #include "phase.h" -class StackLevelSetter : public Phase +class StackLevelSetter final : public Phase { public: StackLevelSetter(Compiler* compiler);