Skip to content

JIT: make global morph its own phase #94185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AndyAyersMS merged 1 commit into from
Oct 31, 2023
Merged

JIT: make global morph its own phase #94185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions src/coreclr/jit/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4747,9 +4747,10 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl

// Morph the trees in all the blocks of the method
//
auto morphGlobalPhase = [this]() {
unsigned prevBBCount = fgBBcount;
fgMorphBlocks();
unsigned const preMorphBBCount = fgBBcount;
DoPhase(this, PHASE_MORPH_GLOBAL, &Compiler::fgMorphBlocks);

auto postMorphPhase = [this]() {

// Fix any LclVar annotations on discarded struct promotion temps for implicit by-ref args
fgMarkDemotedImplicitByRefArgs();
Expand All @@ -4765,16 +4766,17 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
compCurBB = nullptr;
#endif // DEBUG

// If we needed to create any new BasicBlocks then renumber the blocks
if (fgBBcount > prevBBCount)
{
fgRenumberBlocks();
}

// Enable IR checks
activePhaseChecks |= PhaseChecks::CHECK_IR;
};
DoPhase(this, PHASE_MORPH_GLOBAL, morphGlobalPhase);
DoPhase(this, PHASE_POST_MORPH, postMorphPhase);

// If we needed to create any new BasicBlocks then renumber the blocks
//
if (fgBBcount > preMorphBBCount)
{
fgRenumberBlocks();
}

// GS security checks for unsafe buffers
//
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4797,8 +4797,9 @@ class Compiler

FoldResult fgFoldConditional(BasicBlock* block);

PhaseStatus fgMorphBlocks();
void fgMorphBlock(BasicBlock* block);
void fgMorphStmts(BasicBlock* block);
void fgMorphBlocks();

void fgMergeBlockReturn(BasicBlock* block);

Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compphases.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ CompPhaseNameMacro(PHASE_IMPBYREF_COPY_OMISSION, "Identify candidates for im
CompPhaseNameMacro(PHASE_MORPH_IMPBYREF, "Morph - ByRefs", false, -1, false)
CompPhaseNameMacro(PHASE_PROMOTE_STRUCTS, "Morph - Promote Structs", false, -1, false)
CompPhaseNameMacro(PHASE_MORPH_GLOBAL, "Morph - Global", false, -1, false)
CompPhaseNameMacro(PHASE_POST_MORPH, "Post-Morph", false, -1, false)
CompPhaseNameMacro(PHASE_MORPH_END, "Morph - Finish", false, -1, true)
CompPhaseNameMacro(PHASE_GS_COOKIE, "GS Cookie", false, -1, false)
CompPhaseNameMacro(PHASE_COMPUTE_EDGE_WEIGHTS, "Compute edge weights (1, false)",false, -1, false)
Expand Down
143 changes: 67 additions & 76 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13619,13 +13619,12 @@ void Compiler::fgMorphStmtBlockOps(BasicBlock* block, Statement* stmt)
}
}

/*****************************************************************************
*
* Morph the statements of the given block.
* This function should be called just once for a block. Use fgMorphBlockStmt()
* for reentrant calls.
*/

//------------------------------------------------------------------------
// fgMorphStmts: Morph all statements in a block
//
// Arguments:
// block - block in question
//
void Compiler::fgMorphStmts(BasicBlock* block)
{
fgRemoveRestOfBlock = false;
Expand Down Expand Up @@ -13809,36 +13808,65 @@ void Compiler::fgMorphStmts(BasicBlock* block)
fgRemoveRestOfBlock = false;
}

/*****************************************************************************
*
* Morph the blocks of the method.
* Returns true if the basic block list is modified.
* This function should be called just once.
*/

void Compiler::fgMorphBlocks()
//------------------------------------------------------------------------
// fgMorphBlock: Morph a basic block
//
// Arguments:
// block - block in question
//
void Compiler::fgMorphBlock(BasicBlock* block)
{
#ifdef DEBUG
if (verbose)
JITDUMP("\nMorphing " FMT_BB "\n", block->bbNum);

if (optLocalAssertionProp)
{
printf("\n*************** In fgMorphBlocks()\n");
// Clear out any currently recorded assertion candidates
// before processing each basic block,
// also we must handle QMARK-COLON specially
//
optAssertionReset(0);
}
#endif

/* Since fgMorphTree can be called after various optimizations to re-arrange
* the nodes we need a global flag to signal if we are during the one-pass
* global morphing */
// Make the current basic block address available globally.
compCurBB = block;

fgGlobalMorph = true;
// Process all statement trees in the basic block.
fgMorphStmts(block);

// Do we need to merge the result of this block into a single return block?
if (block->KindIs(BBJ_RETURN) && ((block->bbFlags & BBF_HAS_JMP) == 0))
{
if ((genReturnBB != nullptr) && (genReturnBB != block))
{
fgMergeBlockReturn(block);
}
}

compCurBB = nullptr;
}

//------------------------------------------------------------------------
// fgMorphBlocks: Morph all blocks in the method
//
// Returns:
// Suitable phase status.
//
// Note:
// Morph almost always changes IR, so we don't actually bother to
// track if it made any changees.
//
PhaseStatus Compiler::fgMorphBlocks()
{
// This is the one and only global morph phase
//
fgGlobalMorph = true;

// Local assertion prop is enabled if we are optimized
//
optLocalAssertionProp = opts.OptimizationEnabled();

if (optLocalAssertionProp)
{
//
// Initialize for local assertion prop
//
optAssertionInit(true);
Expand All @@ -13862,53 +13890,16 @@ void Compiler::fgMorphBlocks()
fgEnsureFirstBBisScratch();
}

/*-------------------------------------------------------------------------
* Process all basic blocks in the function
*/

BasicBlock* block = fgFirstBB;
noway_assert(block);

do
// Morph all blocks.
//
// Note morph can add blocks downstream from the current block,
// and alter (but not null out) the current block's bbNext;
// this iterator ensures they all get visited.
Comment on lines +13895 to +13897
Copy link
Copy Markdown
Contributor

@BruceForstall BruceForstall Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the iterator load the bbNext before the iteration of the loop? If the callee can adjust the blocks, maybe we shouldn't use the for-based iterator here?

Copy link
Copy Markdown
Member Author

@AndyAyersMS AndyAyersMS Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem to cache anything...? Also that comment is a bit stale; with other changes I've made recently morph won't add blocks or (I suspect) alter bbNext anymore.

Copy link
Copy Markdown
Contributor

@BruceForstall BruceForstall Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it only caches the end-point of the iteration. But still, any change to the block list seems risky to depend on what the C++ compiler is going to do.

Copy link
Copy Markdown
Member Author

@AndyAyersMS AndyAyersMS Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the BasicBlockRangeList form sure... but this is going via BasicBlockSimpleList which does't cache an endpoint.

Copy link
Copy Markdown
Member Author

@AndyAyersMS AndyAyersMS Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BruceForstall still ok with merging? Or would you rather I change the iteration back?

Copy link
Copy Markdown
Contributor

@BruceForstall BruceForstall Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's fine.

//
for (BasicBlock* block : Blocks())
{
#ifdef DEBUG
if (verbose)
{
printf("\nMorphing " FMT_BB " of '%s'\n", block->bbNum, info.compFullName);
}
#endif

if (optLocalAssertionProp)
{
//
// Clear out any currently recorded assertion candidates
// before processing each basic block,
// also we must handle QMARK-COLON specially
//
optAssertionReset(0);
}

// Make the current basic block address available globally.
compCurBB = block;

// Process all statement trees in the basic block.
fgMorphStmts(block);

// Do we need to merge the result of this block into a single return block?
if (block->KindIs(BBJ_RETURN) && ((block->bbFlags & BBF_HAS_JMP) == 0))
{
if ((genReturnBB != nullptr) && (genReturnBB != block))
{
fgMergeBlockReturn(block);
}
}

block = block->Next();
} while (block != nullptr);

// We are done with the global morphing phase
fgGlobalMorph = false;
compCurBB = nullptr;
fgMorphBlock(block);
}

// Under OSR, we no longer need to specially protect the original method entry
//
Expand All @@ -13924,12 +13915,12 @@ void Compiler::fgMorphBlocks()
fgEntryBB = nullptr;
}

#ifdef DEBUG
if (verboseTrees)
{
fgDispBasicBlocks(true);
}
#endif
// We are done with the global morphing phase
//
fgGlobalMorph = false;
compCurBB = nullptr;

return PhaseStatus::MODIFIED_EVERYTHING;
}

//------------------------------------------------------------------------
Expand Down