JIT: rework phase objects so we can use them more widely (#31808)
authorAndy Ayers <andya@microsoft.com>
Thu, 6 Feb 2020 02:34:15 +0000 (18:34 -0800)
committerGitHub <noreply@github.com>
Thu, 6 Feb 2020 02:34:15 +0000 (18:34 -0800)
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.

15 files changed:
src/coreclr/src/jit/CMakeLists.txt
src/coreclr/src/jit/compiler.cpp
src/coreclr/src/jit/compiler.h
src/coreclr/src/jit/compiler.hpp
src/coreclr/src/jit/error.cpp
src/coreclr/src/jit/jit.settings.targets
src/coreclr/src/jit/jittelemetry.cpp
src/coreclr/src/jit/lower.cpp
src/coreclr/src/jit/lower.h
src/coreclr/src/jit/objectalloc.h
src/coreclr/src/jit/phase.cpp [new file with mode: 0644]
src/coreclr/src/jit/phase.h
src/coreclr/src/jit/rationalize.h
src/coreclr/src/jit/stacklevelsetter.cpp
src/coreclr/src/jit/stacklevelsetter.h

index bc91b7b..b63e08a 100644 (file)
@@ -67,6 +67,7 @@ set( JIT_SOURCES
   objectalloc.cpp
   optcse.cpp
   optimizer.cpp
+  phase.cpp
   rangecheck.cpp
   rationalize.cpp
   regalloc.cpp
index 35d0c0d..f4bc154 100644 (file)
@@ -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())
index 098a170..33a1859 100644 (file)
@@ -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).
index 429a583..d0de0f1 100644 (file)
@@ -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
 
index dd020e5..90a2a76 100644 (file)
@@ -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
 
index 9c567c0..666b409 100644 (file)
@@ -93,6 +93,7 @@
         <CppCompile Include="..\LSRA.cpp" />
         <CppCompile Include="..\lsrabuild.cpp" />
         <CppCompile Include="..\codegenlinear.cpp" />
+        <CppCompile Include="..\phase.cpp" />
     </ItemGroup>
     <ItemGroup Condition="'$(TargetArch)'=='i386'">
         <CppCompile Include="..\emitXArch.cpp" />
index 021fb45..3861789 100644 (file)
@@ -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();
index bf47b52..a88f895 100644 (file)
@@ -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
index 1ad3a72..3fef773 100644 (file)
@@ -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);
index 76f5ec9..fa4aff5 100644 (file)
@@ -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 (file)
index 0000000..1d5b055
--- /dev/null
@@ -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);
+}
index 2077f94..646a8a2 100644 (file)
@@ -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 <functional> here, but seemingly can't.
+//
+template <typename A>
+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 <typename A>
+void DoPhase(Compiler* _compiler, Phases _phase, A _action)
+{
+    ActionPhase<A> 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_ */
index d396ac8..2b35a49 100644 (file)
@@ -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;
index 4de2535..15b77b2 100644 (file)
@@ -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))
index 984cf54..4ee9b99 100644 (file)
@@ -7,7 +7,7 @@
 #include "compiler.h"
 #include "phase.h"
 
-class StackLevelSetter : public Phase
+class StackLevelSetter final : public Phase
 {
 public:
     StackLevelSetter(Compiler* compiler);