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.

Author: AndyAyersMS

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

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
 
@@ -4214,6 +4219,37 @@ void Compiler::compFunctionTraceEnd(void* methodCodePtr, ULONG methodCodeSize, b
 #endif // DEBUG
 }
 
+//------------------------------------------------------------------------
+// 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
 //
@@ -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())

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 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 
 #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 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
     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).

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
 

src/coreclr/src/jit/error.cpp

@@ -272,19 +272,21 @@ 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
 
 #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
 

src/coreclr/src/jit/jit.settings.targets

@@ -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" />

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();

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

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);

src/coreclr/src/jit/objectalloc.h

@@ -76,15 +76,12 @@ class ObjectAllocator final : public Phase
 //===============================================================================
 
 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();