Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/coreclr/jit/block.h
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,7 @@ enum BasicBlockFlags : unsigned __int64
BBF_TAILCALL_SUCCESSOR = MAKE_BBFLAG(40), // BB has pred that has potential tail call
BBF_RECURSIVE_TAILCALL = MAKE_BBFLAG(41), // Block has recursive tailcall that may turn into a loop
BBF_NO_CSE_IN = MAKE_BBFLAG(42), // Block should kill off any incoming CSE
BBF_CAN_ADD_PRED = MAKE_BBFLAG(43), // Ok to add pred edge to this block, even when "safe" edge creation disabled

// The following are sets of flags.

Expand Down
7 changes: 4 additions & 3 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -3215,10 +3215,11 @@ class Compiler
//=========================================================================
// BasicBlock functions
#ifdef DEBUG
// This is a debug flag we will use to assert when creating block during codegen
// as this interferes with procedure splitting. If you know what you're doing, set
// it to true before creating the block. (DEBUG only)
// When false, assert when creating a new basic block.
bool fgSafeBasicBlockCreation;

// When false, assert when creating a new flow edge
bool fgSafeFlowEdgeCreation;
#endif

/*
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/fgbasic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ void Compiler::fgInit()

#ifdef DEBUG
fgSafeBasicBlockCreation = true;
fgSafeFlowEdgeCreation = true;
#endif // DEBUG

fgLocalVarLivenessDone = false;
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/jit/fgflow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,11 @@ FlowEdge* Compiler::fgAddRefPred(BasicBlock* block, BasicBlock* blockPred, FlowE
}
else
{
// Create a new edge
//
// We may be disallowing edge creation, except for edges targeting special blocks.
//
assert(fgSafeFlowEdgeCreation || ((block->bbFlags & BBF_CAN_ADD_PRED) != 0));

#if MEASURE_BLOCK_SIZE
genFlowNodeCnt += 1;
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/flowgraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2228,7 +2228,7 @@ class MergedReturns
// Add 'return' expression to the return block
comp->fgNewStmtAtEnd(newReturnBB, returnExpr);
// Flag that this 'return' was generated by return merging so that subsequent
// return block morhping will know to leave it alone.
// return block merging will know to leave it alone.
returnExpr->gtFlags |= GTF_RET_MERGED;

#ifdef DEBUG
Expand Down
70 changes: 63 additions & 7 deletions src/coreclr/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7472,7 +7472,6 @@ void Compiler::fgMorphRecursiveFastTailCallIntoLoop(BasicBlock* block, GenTreeCa
//------------------------------------------------------------------------------
// fgAssignRecursiveCallArgToCallerParam : Assign argument to a recursive call to the corresponding caller parameter.
//
//
// Arguments:
// arg - argument to assign
// late - whether to use early or late arg
Expand Down Expand Up @@ -13892,13 +13891,70 @@ PhaseStatus Compiler::fgMorphBlocks()

// 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.
//
for (BasicBlock* block : Blocks())
if (!optLocalAssertionProp)
{
fgMorphBlock(block);
// If we aren't optimizing, we just morph in normal bbNext order.
//
// 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.
//
for (BasicBlock* block : Blocks())
{
fgMorphBlock(block);
}
}
else
{
// We are optimizing. Process in RPO.
//
fgRenumberBlocks();
EnsureBasicBlockEpoch();
fgComputeEnterBlocksSet();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How much of the cost comes from having to do the above setup calls before even computing the order?
When I've tried to compute the order before it wasn't clear to me why I would need to renumber blocks to do that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can grab a new profile, but the old one is probably pretty accurate: see #86822 (comment)

Renumbering likely isn't needed and not renumbering should save a bit of TP. So let me try that.

Also looking back at that early PR reminded me that I should take a look at if SSA's topological sort, it might be more efficient.

Copy link
Copy Markdown
Member

@jakobbotsch jakobbotsch Nov 1, 2023

Choose a reason for hiding this comment

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

It's also possible we can make DfsBlockEntry cheaper in the same way as #86839 (even though GetSucc looks cheap, it would probably still help a bit).

It also confuses me a bit that we have multiple notions of start nodes with fgEnterBlks and fgDomFindStartNodes. I assume the nodes found exclusively by fgDomFindStartNodes are unreachable (?) so ideally we would get rid of them earlier, but even if not, isn't it optimizable by using bbRefs? If not, then I think using VisitRegularSuccs would be more efficient.

Anyway, don't feel the need to address it in this PR -- but might be a couple of avenues of future cleanup/improvements.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Seems like for now at least renumbering is needed. Let me add some notes to #93246 about making all this more efficient. I'll come back to that after I get the assertion prop part working.

fgDfsReversePostorder();

// Disallow general creation of new blocks or edges as it
// would invalidate RPO.
//
// Removal of edges, or altering dup counts, is OK.
//
INDEBUG(fgSafeBasicBlockCreation = false;);
INDEBUG(fgSafeFlowEdgeCreation = false;);

// Allow edge creation to genReturnBB (target of return merging)
// and the scratch block successor (target for tail call to loop).
// This will also disallow dataflow into these blocks.
//
if (genReturnBB != nullptr)
{
genReturnBB->bbFlags |= BBF_CAN_ADD_PRED;
}
if (fgFirstBBisScratch())
{
fgFirstBB->Next()->bbFlags |= BBF_CAN_ADD_PRED;
}

Comment on lines +13924 to +13936
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's unfortunate we need these special cases (and the new block flag), but I understand you tried to avoid it and this is a compromise.

unsigned const bbNumMax = fgBBNumMax;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you capture fgBBNumMax because the number of blocks can change during morph? But you've disabled block creation. Do you want an assert after the for loop to assert no new blocks were created?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah this is a vestige of my earlier versions where the number of blocks could change, before I removed all the bits that create new blocks.

You think this should have assert(fgBBNumMax == bbNumMax)?

If so, sure. I can add that in my next PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You think this should have assert(fgBBNumMax == bbNumMax)?

Right. That would clarify the assumption that the set of blocks doesn't change across this loop.

for (unsigned i = 1; i <= bbNumMax; i++)
{
BasicBlock* const block = fgBBReversePostorder[i];
fgMorphBlock(block);
}

// Re-enable block and edge creation, and revoke
// special treatment of genReturnBB and the "first" bb
//
INDEBUG(fgSafeBasicBlockCreation = true;);
INDEBUG(fgSafeFlowEdgeCreation = true;);

if (genReturnBB != nullptr)
{
genReturnBB->bbFlags &= ~BBF_CAN_ADD_PRED;
}
if (fgFirstBBisScratch())
{
fgFirstBB->Next()->bbFlags &= ~BBF_CAN_ADD_PRED;
}
}

// Under OSR, we no longer need to specially protect the original method entry
Expand Down