Skip to content

Commit

Permalink
JIT: let instrumentor decide which blocks to process (#47597)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@AndyAyersMS
AndyAyersMS authored Jan 29, 2021
1 parent 361f24d commit 42bc286
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 38 deletions.
2 changes: 1 addition & 1 deletion src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
57 changes: 33 additions & 24 deletions src/coreclr/jit/fgprofile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,10 @@ class Instrumentor
}

public:
virtual bool ShouldProcess(BasicBlock* block)
{
return false;
}
virtual void Prepare()
{
}
Expand Down Expand Up @@ -267,14 +271,18 @@ class NonInstrumentor : public Instrumentor

//------------------------------------------------------------------------
// BlockCountInstrumentor: instrumentor that adds a counter to each
// basic block
// non-internal imported basic block
//
class BlockCountInstrumentor : public Instrumentor
{
public:
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;
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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());

Expand Down Expand Up @@ -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.
Expand All @@ -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(),
Expand All @@ -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.
Expand All @@ -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)
Expand Down
20 changes: 7 additions & 13 deletions src/coreclr/jit/phase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 42bc286

Please sign in to comment.