From 0746971bc755cb0804e2c57e9511c7cf12edc65d Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 25 Jan 2021 19:34:37 -0800 Subject: [PATCH] JIT: let instrumentor decide which blocks to process Forthcoming edge-based instrumentation will need to handle some BBF_INTERNAL blocks, so update main processing logic to accomodate this. Also have `fgInstrument` return proper phase status, so we get after-phase dumping if any instrumentation is added. --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/fgprofile.cpp | 57 ++++++++++++++++++++--------------- src/coreclr/jit/phase.cpp | 20 +++++------- 3 files changed, 41 insertions(+), 38 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index ca3d5ace3bc060..ca7057faadf140 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5561,7 +5561,7 @@ class Compiler bool fgHaveProfileData(); void fgComputeProfileScale(); bool fgGetProfileWeightForBasicBlock(IL_OFFSET offset, BasicBlock::weight_t* weight); - void fgInstrumentMethod(); + PhaseStatus fgInstrumentMethod(); public: // fgIsUsingProfileWeights - returns true if we have real profile data for this method diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index d9a5bb16b72cf9..681b89f72678ba 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -229,6 +229,10 @@ class Instrumentor } public: + virtual bool ShouldProcess(BasicBlock* block) + { + return false; + } virtual void Prepare() { } @@ -267,7 +271,7 @@ class NonInstrumentor : public Instrumentor //------------------------------------------------------------------------ // BlockCountInstrumentor: instrumentor that adds a counter to each -// basic block +// non-internal imported basic block // class BlockCountInstrumentor : public Instrumentor { @@ -275,6 +279,10 @@ class BlockCountInstrumentor : public Instrumentor BlockCountInstrumentor(Compiler* comp) : Instrumentor(comp) { } + bool ShouldProcess(BasicBlock* block) override + { + return ((block->bbFlags & (BBF_INTERNAL | BBF_IMPORTED)) == BBF_IMPORTED); + } void Prepare() override; void BuildSchemaElements(BasicBlock* block, Schema& schema) override; void Instrument(BasicBlock* block, Schema& schema, BYTE* profileMemory) override; @@ -302,6 +310,7 @@ void BlockCountInstrumentor::Prepare() // // Arguments: // block -- block to instrument +// schema -- schema that we're building // void BlockCountInstrumentor::BuildSchemaElements(BasicBlock* block, Schema& schema) { @@ -621,6 +630,10 @@ class ClassProbeInstrumentor : public Instrumentor ClassProbeInstrumentor(Compiler* comp) : Instrumentor(comp) { } + bool ShouldProcess(BasicBlock* block) override + { + return ((block->bbFlags & (BBF_INTERNAL | BBF_IMPORTED)) == BBF_IMPORTED); + } void Prepare() override; void BuildSchemaElements(BasicBlock* block, Schema& schema) override; void Instrument(BasicBlock* block, Schema& schema, BYTE* profileMemory) override; @@ -648,6 +661,7 @@ void ClassProbeInstrumentor::Prepare() // // Arguments: // block -- block to instrument +// schema -- schema that we're building // void ClassProbeInstrumentor::BuildSchemaElements(BasicBlock* block, Schema& schema) { @@ -707,7 +721,7 @@ void ClassProbeInstrumentor::Instrument(BasicBlock* block, Schema& schema, BYTE* // ClassProbeInstrumentor::SuppressProbes: clean up if we're not instrumenting // // Notes: -// Currently we're hijacking the gtCallStubAddre of the call node to hold +// Currently we're hijacking the gtCallStubAddr of the call node to hold // a pointer to the profile candidate info. // // We must undo this, if not instrumenting. @@ -737,6 +751,9 @@ void ClassProbeInstrumentor::SuppressProbes() //------------------------------------------------------------------------ // fgInstrumentMethod: add instrumentation probes to the method // +// Returns: +// appropriate phase status +// // Note: // // By default this instruments each non-internal block with @@ -747,7 +764,7 @@ void ClassProbeInstrumentor::SuppressProbes() // Probe structure is described by a schema array, which is created // here based on flowgraph and IR structure. // -void Compiler::fgInstrumentMethod() +PhaseStatus Compiler::fgInstrumentMethod() { noway_assert(!compIsForInlining()); @@ -775,20 +792,15 @@ void Compiler::fgInstrumentMethod() Schema schema(getAllocator(CMK_Pgo)); for (BasicBlock* block = fgFirstBB; (block != nullptr); block = block->bbNext) { - // Skip internal and un-imported blocks. - // - if ((block->bbFlags & BBF_IMPORTED) == 0) + if (countInst->ShouldProcess(block)) { - continue; + countInst->BuildSchemaElements(block, schema); } - if ((block->bbFlags & BBF_INTERNAL) == BBF_INTERNAL) + if (classInst->ShouldProcess(block)) { - continue; + classInst->BuildSchemaElements(block, schema); } - - countInst->BuildSchemaElements(block, schema); - classInst->BuildSchemaElements(block, schema); } // Verify we created schema for the calls needing class probes. @@ -802,7 +814,7 @@ void Compiler::fgInstrumentMethod() if ((JitConfig.JitMinimalProfiling() > 0) && (countInst->SchemaCount() == 1) && (classInst->SchemaCount() == 0)) { JITDUMP("Not instrumenting method: only one counter, and no class probes\n"); - return; + return PhaseStatus::MODIFIED_NOTHING; } JITDUMP("Instrumenting method: %d count probes and %d class probes\n", countInst->SchemaCount(), @@ -826,34 +838,29 @@ void Compiler::fgInstrumentMethod() if (res != E_NOTIMPL) { noway_assert(!"Error: unexpected hresult from allocPgoInstrumentationBySchema"); - return; + return PhaseStatus::MODIFIED_NOTHING; } // Do any cleanup we might need to do... // countInst->SuppressProbes(); classInst->SuppressProbes(); - return; + return PhaseStatus::MODIFIED_NOTHING; } // Add the instrumentation code // for (BasicBlock* block = fgFirstBB; (block != nullptr); block = block->bbNext) { - // Skip internal and un-imported blocks. - // - if ((block->bbFlags & BBF_IMPORTED) == 0) + if (countInst->ShouldProcess(block)) { - continue; + countInst->Instrument(block, schema, profileMemory); } - if ((block->bbFlags & BBF_INTERNAL) == BBF_INTERNAL) + if (classInst->ShouldProcess(block)) { - continue; + classInst->Instrument(block, schema, profileMemory); } - - countInst->Instrument(block, schema, profileMemory); - classInst->Instrument(block, schema, profileMemory); } // Verify we instrumented everthing we created schemas for. @@ -866,6 +873,8 @@ void Compiler::fgInstrumentMethod() // countInst->InstrumentMethodEntry(schema, profileMemory); classInst->InstrumentMethodEntry(schema, profileMemory); + + return PhaseStatus::MODIFIED_EVERYTHING; } bool flowList::setEdgeWeightMinChecked(BasicBlock::weight_t newWeight, BasicBlock::weight_t slop, bool* wbUsedSlop) diff --git a/src/coreclr/jit/phase.cpp b/src/coreclr/jit/phase.cpp index b2387101e9b22d..3a78ef3b2b3def 100644 --- a/src/coreclr/jit/phase.cpp +++ b/src/coreclr/jit/phase.cpp @@ -157,19 +157,13 @@ void Phase::PostPhase(PhaseStatus status) // well as the new-style phases that have been updated to return // PhaseStatus from their DoPhase methods. // - static Phases s_allowlist[] = {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_MORPH_GLOBAL, - PHASE_BUILD_SSA, - PHASE_RATIONALIZE, - PHASE_LOWERING, + static Phases s_allowlist[] = {PHASE_IMPORTATION, PHASE_IBCINSTR, + 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_MORPH_GLOBAL, PHASE_BUILD_SSA, + PHASE_RATIONALIZE, PHASE_LOWERING, PHASE_STACK_LEVEL_SETTER}; if (madeChanges)