Skip to content
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

Jump to bottom

Reduce differences between successor iterators #93445

Merged
merged 2 commits into from
Oct 13, 2023
Merged

Reduce differences between successor iterators #93445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0410dd4
Reduce differences between successor iterators
BruceForstall Oct 12, 2023
cf4e02b
Review comments
BruceForstall Oct 13, 2023
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
24 changes: 22 additions & 2 deletions src/coreclr/jit/block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1081,14 +1081,13 @@ unsigned BasicBlock::NumSucc() const
{
case BBJ_THROW:
case BBJ_RETURN:
case BBJ_EHFINALLYRET:
case BBJ_EHFAULTRET:
case BBJ_EHFILTERRET:
return 0;

case BBJ_CALLFINALLY:
case BBJ_ALWAYS:
case BBJ_EHCATCHRET:
case BBJ_EHFILTERRET:
case BBJ_LEAVE:
case BBJ_NONE:
return 1;
Expand All @@ -1103,6 +1102,23 @@ unsigned BasicBlock::NumSucc() const
return 2;
}

case BBJ_EHFINALLYRET:
// We may call this method before we realize we have invalid IL. Tolerate.
//
if (!hasHndIndex())
{
return 0;
}

// We may call this before we've computed the BBJ_EHFINALLYRET successors in the importer. Tolerate.
//
if (bbJumpEhf == nullptr)
{
return 0;
}

return bbJumpEhf->bbeCount;

case BBJ_SWITCH:
return bbJumpSwt->bbsCount;

Expand All @@ -1128,6 +1144,7 @@ BasicBlock* BasicBlock::GetSucc(unsigned i) const
case BBJ_CALLFINALLY:
case BBJ_ALWAYS:
case BBJ_EHCATCHRET:
case BBJ_EHFILTERRET:
case BBJ_LEAVE:
return bbJumpDest;

Expand All @@ -1146,6 +1163,9 @@ BasicBlock* BasicBlock::GetSucc(unsigned i) const
return bbJumpDest;
}

case BBJ_EHFINALLYRET:
return bbJumpEhf->bbeSuccs[i];

case BBJ_SWITCH:
return bbJumpSwt->bbsDstTab[i];

Expand Down
29 changes: 18 additions & 11 deletions src/coreclr/jit/block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,15 +887,7 @@ struct BasicBlock : private LIR::Range
// GetSucc() without a Compiler*.
//
// The behavior of NumSucc()/GetSucc() is different when passed a Compiler* for blocks that end in:
// (1) BBJ_EHFINALLYRET (a return from a finally block)
// (2) BBJ_EHFILTERRET (a return from EH filter block)
// (3) BBJ_SWITCH
//
// For BBJ_EHFINALLYRET, if no Compiler* is passed, then the block is considered to have no
// successor. If Compiler* is passed, we use the actual successors.
//
// Similarly, BBJ_EHFILTERRET blocks are assumed to have no successors if Compiler* is not passed; if
// Compiler* is passed, NumSucc/GetSucc yields the first block of the try block's handler.
// (1) BBJ_SWITCH
//
// For BBJ_SWITCH, if Compiler* is not passed, then all switch successors are returned. If Compiler*
// is passed, then only unique switch successors are returned; the duplicate successors are omitted.
Expand Down Expand Up @@ -1751,9 +1743,7 @@ inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block)
{
case BBJ_THROW:
case BBJ_RETURN:
case BBJ_EHFINALLYRET:
case BBJ_EHFAULTRET:
case BBJ_EHFILTERRET:
// We don't need m_succs.
m_begin = nullptr;
m_end = nullptr;
Expand All @@ -1762,6 +1752,7 @@ inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block)
case BBJ_CALLFINALLY:
case BBJ_ALWAYS:
case BBJ_EHCATCHRET:
case BBJ_EHFILTERRET:
case BBJ_LEAVE:
m_succs[0] = block->GetJumpDest();
m_begin = &m_succs[0];
Expand Down Expand Up @@ -1791,6 +1782,22 @@ inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block)
}
break;

case BBJ_EHFINALLYRET:
// We don't use the m_succs in-line data; use the existing successor table in the block.
// We must tolerate iterating successors early in the system, before EH_FINALLYRET successors have
// been computed.
if (block->GetJumpEhf() == nullptr)
{
m_begin = nullptr;
m_end = nullptr;
}
else
{
m_begin = block->GetJumpEhf()->bbeSuccs;
m_end = block->GetJumpEhf()->bbeSuccs + block->GetJumpEhf()->bbeCount;
}
break;

case BBJ_SWITCH:
// We don't use the m_succs in-line data for switches; use the existing jump table in the block.
assert(block->GetJumpSwt() != nullptr);
Expand Down
12 changes: 2 additions & 10 deletions src/coreclr/jit/compiler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,12 +616,6 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func)
{
switch (bbJumpKind)
{
case BBJ_EHFILTERRET:
RETURN_ON_ABORT(func(bbJumpDest));
RETURN_ON_ABORT(VisitEHSuccessors(comp, this, func));
RETURN_ON_ABORT(VisitSuccessorEHSuccessors(comp, this, bbJumpDest, func));
break;

case BBJ_EHFINALLYRET:
for (unsigned i = 0; i < bbJumpEhf->bbeCount; i++)
{
Expand All @@ -639,6 +633,7 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func)

case BBJ_CALLFINALLY:
case BBJ_EHCATCHRET:
case BBJ_EHFILTERRET:
case BBJ_LEAVE:
RETURN_ON_ABORT(func(bbJumpDest));
RETURN_ON_ABORT(VisitEHSuccessors(comp, this, func));
Expand Down Expand Up @@ -728,10 +723,6 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func)
{
switch (bbJumpKind)
{
case BBJ_EHFILTERRET:
RETURN_ON_ABORT(func(bbJumpDest));
break;

case BBJ_EHFINALLYRET:
for (unsigned i = 0; i < bbJumpEhf->bbeCount; i++)
{
Expand All @@ -741,6 +732,7 @@ BasicBlockVisit BasicBlock::VisitRegularSuccs(Compiler* comp, TFunc func)

case BBJ_CALLFINALLY:
case BBJ_EHCATCHRET:
case BBJ_EHFILTERRET:
case BBJ_LEAVE:
case BBJ_ALWAYS:
RETURN_ON_ABORT(func(bbJumpDest));
Expand Down
15 changes: 11 additions & 4 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11471,7 +11471,9 @@ void Compiler::impImportBlock(BasicBlock* block)
// This will re-import all the successors of block (as well as each of their predecessors)
impReimportSpillClique(block);

// For blocks that haven't been imported yet, we still need to mark them as pending import.
// We don't expect to see BBJ_EHFILTERRET here.
assert(!block->KindIs(BBJ_EHFILTERRET));

for (BasicBlock* const succ : block->Succs())
{
if ((succ->bbFlags & BBF_IMPORTED) == 0)
Expand All @@ -11484,10 +11486,15 @@ void Compiler::impImportBlock(BasicBlock* block)
{
// otherwise just import the successors of block

/* Does this block jump to any other blocks? */
for (BasicBlock* const succ : block->Succs())
// Does this block jump to any other blocks?
// Filter successor from BBJ_EHFILTERRET have already been handled above in the call
// to impVerifyEHBlock().
if (!block->KindIs(BBJ_EHFILTERRET))
{
impImportBlockPending(succ);
for (BasicBlock* const succ : block->Succs())
{
impImportBlockPending(succ);
}
}
}
}
Expand Down
Loading