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

JIT: Use successor edges instead of block targets for BBJ_SWITCH #98671

Merged
merged 7 commits into from
Feb 20, 2024
Merged

JIT: Use successor edges instead of block targets for BBJ_SWITCH #98671

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
407cabb
Update BBswtDesc
amanasifkhalid Feb 14, 2024
00ee187
Use new fgRemoveRefPred
amanasifkhalid Feb 19, 2024
f0b48ee
Rename optRedirectBlock
amanasifkhalid Feb 19, 2024
0d043a3
Fix jitstress
amanasifkhalid Feb 19, 2024
ba23360
Fix build
amanasifkhalid Feb 19, 2024
0ee452d
Fix fgRemoveRefPred usage
amanasifkhalid Feb 20, 2024
eabe3e8
Rename to optSetMappedBlockTargets
amanasifkhalid Feb 20, 2024
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
10 changes: 5 additions & 5 deletions src/coreclr/jit/block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,12 +722,12 @@ void BasicBlock::dspKind() const
{
printf(" ->");

const unsigned jumpCnt = bbSwtTargets->bbsCount;
BasicBlock** const jumpTab = bbSwtTargets->bbsDstTab;
const unsigned jumpCnt = bbSwtTargets->bbsCount;
FlowEdge** const jumpTab = bbSwtTargets->bbsDstTab;

for (unsigned i = 0; i < jumpCnt; i++)
{
printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(jumpTab[i]));
printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(jumpTab[i]->getDestinationBlock()));

const bool isDefault = bbSwtTargets->bbsHasDefault && (i == jumpCnt - 1);
if (isDefault)
Expand Down Expand Up @@ -1217,7 +1217,7 @@ BasicBlock* BasicBlock::GetSucc(unsigned i) const
return bbEhfTargets->bbeSuccs[i]->getDestinationBlock();

case BBJ_SWITCH:
return bbSwtTargets->bbsDstTab[i];
return bbSwtTargets->bbsDstTab[i]->getDestinationBlock();

default:
unreached();
Expand Down Expand Up @@ -1774,7 +1774,7 @@ BBswtDesc::BBswtDesc(Compiler* comp, const BBswtDesc* other)
{
// Allocate and fill in a new dst tab
//
bbsDstTab = new (comp, CMK_BasicBlock) BasicBlock*[bbsCount];
bbsDstTab = new (comp, CMK_FlowEdge) FlowEdge*[bbsCount];
for (unsigned i = 0; i < bbsCount; i++)
{
bbsDstTab[i] = other->bbsDstTab[i];
Expand Down
12 changes: 7 additions & 5 deletions src/coreclr/jit/block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1840,8 +1840,8 @@ class BasicBlockRangeList
//
struct BBswtDesc
{
BasicBlock** bbsDstTab; // case label table address
unsigned bbsCount; // count of cases (includes 'default' if bbsHasDefault)
FlowEdge** bbsDstTab; // case label table address
unsigned bbsCount; // count of cases (includes 'default' if bbsHasDefault)

// Case number and likelihood of most likely case
// (only known with PGO, only valid if bbsHasDominantCase is true)
Expand All @@ -1867,7 +1867,7 @@ struct BBswtDesc
bbsCount--;
}

BasicBlock* getDefault()
FlowEdge* getDefault()
{
assert(bbsHasDefault);
assert(bbsCount > 0);
Expand Down Expand Up @@ -1994,8 +1994,10 @@ inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block)
// We don't use the m_succs in-line data for switches; use the existing jump table in the block.
assert(block->bbSwtTargets != nullptr);
assert(block->bbSwtTargets->bbsDstTab != nullptr);
m_begin = block->bbSwtTargets->bbsDstTab;
m_end = block->bbSwtTargets->bbsDstTab + block->bbSwtTargets->bbsCount;
m_beginEdge = block->bbSwtTargets->bbsDstTab;
m_endEdge = block->bbSwtTargets->bbsDstTab + block->bbSwtTargets->bbsCount;

iterateEdges = true;
break;

default:
Expand Down
9 changes: 5 additions & 4 deletions src/coreclr/jit/codegenarm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,17 +650,18 @@ void CodeGen::genJumpTable(GenTree* treeNode)
noway_assert(compiler->compCurBB->KindIs(BBJ_SWITCH));
assert(treeNode->OperGet() == GT_JMPTABLE);

unsigned jumpCount = compiler->compCurBB->GetSwitchTargets()->bbsCount;
BasicBlock** jumpTable = compiler->compCurBB->GetSwitchTargets()->bbsDstTab;
unsigned jmpTabBase;
unsigned jumpCount = compiler->compCurBB->GetSwitchTargets()->bbsCount;
FlowEdge** jumpTable = compiler->compCurBB->GetSwitchTargets()->bbsDstTab;
unsigned jmpTabBase;

jmpTabBase = GetEmitter()->emitBBTableDataGenBeg(jumpCount, false);

JITDUMP("\n J_M%03u_DS%02u LABEL DWORD\n", compiler->compMethodID, jmpTabBase);

for (unsigned i = 0; i < jumpCount; i++)
{
BasicBlock* target = *jumpTable++;
BasicBlock* target = (*jumpTable)->getDestinationBlock();
jumpTable++;
noway_assert(target->HasFlag(BBF_HAS_LABEL));

JITDUMP(" DD L_M%03u_" FMT_BB "\n", compiler->compMethodID, target->bbNum);
Expand Down
11 changes: 6 additions & 5 deletions src/coreclr/jit/codegenarm64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3753,10 +3753,10 @@ void CodeGen::genJumpTable(GenTree* treeNode)
noway_assert(compiler->compCurBB->KindIs(BBJ_SWITCH));
assert(treeNode->OperGet() == GT_JMPTABLE);

unsigned jumpCount = compiler->compCurBB->GetSwitchTargets()->bbsCount;
BasicBlock** jumpTable = compiler->compCurBB->GetSwitchTargets()->bbsDstTab;
unsigned jmpTabOffs;
unsigned jmpTabBase;
unsigned jumpCount = compiler->compCurBB->GetSwitchTargets()->bbsCount;
FlowEdge** jumpTable = compiler->compCurBB->GetSwitchTargets()->bbsDstTab;
unsigned jmpTabOffs;
unsigned jmpTabBase;

jmpTabBase = GetEmitter()->emitBBTableDataGenBeg(jumpCount, true);

Expand All @@ -3766,7 +3766,8 @@ void CodeGen::genJumpTable(GenTree* treeNode)

for (unsigned i = 0; i < jumpCount; i++)
{
BasicBlock* target = *jumpTable++;
BasicBlock* target = (*jumpTable)->getDestinationBlock();
jumpTable++;
noway_assert(target->HasFlag(BBF_HAS_LABEL));

JITDUMP(" DD L_M%03u_" FMT_BB "\n", compiler->compMethodID, target->bbNum);
Expand Down
11 changes: 6 additions & 5 deletions src/coreclr/jit/codegenloongarch64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2930,10 +2930,10 @@ void CodeGen::genJumpTable(GenTree* treeNode)
noway_assert(compiler->compCurBB->KindIs(BBJ_SWITCH));
assert(treeNode->OperGet() == GT_JMPTABLE);

unsigned jumpCount = compiler->compCurBB->GetSwitchTargets()->bbsCount;
BasicBlock** jumpTable = compiler->compCurBB->GetSwitchTargets()->bbsDstTab;
unsigned jmpTabOffs;
unsigned jmpTabBase;
unsigned jumpCount = compiler->compCurBB->GetSwitchTargets()->bbsCount;
FlowEdge** jumpTable = compiler->compCurBB->GetSwitchTargets()->bbsDstTab;
unsigned jmpTabOffs;
unsigned jmpTabBase;

jmpTabBase = GetEmitter()->emitBBTableDataGenBeg(jumpCount, true);

Expand All @@ -2943,7 +2943,8 @@ void CodeGen::genJumpTable(GenTree* treeNode)

for (unsigned i = 0; i < jumpCount; i++)
{
BasicBlock* target = *jumpTable++;
BasicBlock* target = (*jumpTable)->getDestinationBlock();
jumpTable++;
noway_assert(target->HasFlag(BBF_HAS_LABEL));

JITDUMP(" DD L_M%03u_" FMT_BB "\n", compiler->compMethodID, target->bbNum);
Expand Down
10 changes: 5 additions & 5 deletions src/coreclr/jit/codegenriscv64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2852,10 +2852,10 @@ void CodeGen::genJumpTable(GenTree* treeNode)
noway_assert(compiler->compCurBB->KindIs(BBJ_SWITCH));
assert(treeNode->OperGet() == GT_JMPTABLE);

unsigned jumpCount = compiler->compCurBB->GetSwitchTargets()->bbsCount;
BasicBlock** jumpTable = compiler->compCurBB->GetSwitchTargets()->bbsDstTab;
unsigned jmpTabOffs;
unsigned jmpTabBase;
unsigned jumpCount = compiler->compCurBB->GetSwitchTargets()->bbsCount;
FlowEdge** jumpTable = compiler->compCurBB->GetSwitchTargets()->bbsDstTab;
unsigned jmpTabOffs;
unsigned jmpTabBase;

jmpTabBase = GetEmitter()->emitBBTableDataGenBeg(jumpCount, true);

Expand All @@ -2865,7 +2865,7 @@ void CodeGen::genJumpTable(GenTree* treeNode)

for (unsigned i = 0; i < jumpCount; i++)
{
BasicBlock* target = *jumpTable++;
BasicBlock* target = (*jumpTable)->getDestinationBlock();
noway_assert(target->HasFlag(BBF_HAS_LABEL));

JITDUMP(" DD L_M%03u_" FMT_BB "\n", compiler->compMethodID, target->bbNum);
Expand Down
11 changes: 6 additions & 5 deletions src/coreclr/jit/codegenxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4356,10 +4356,10 @@ void CodeGen::genJumpTable(GenTree* treeNode)
noway_assert(compiler->compCurBB->KindIs(BBJ_SWITCH));
assert(treeNode->OperGet() == GT_JMPTABLE);

unsigned jumpCount = compiler->compCurBB->GetSwitchTargets()->bbsCount;
BasicBlock** jumpTable = compiler->compCurBB->GetSwitchTargets()->bbsDstTab;
unsigned jmpTabOffs;
unsigned jmpTabBase;
unsigned jumpCount = compiler->compCurBB->GetSwitchTargets()->bbsCount;
FlowEdge** jumpTable = compiler->compCurBB->GetSwitchTargets()->bbsDstTab;
unsigned jmpTabOffs;
unsigned jmpTabBase;

jmpTabBase = GetEmitter()->emitBBTableDataGenBeg(jumpCount, true);

Expand All @@ -4369,7 +4369,8 @@ void CodeGen::genJumpTable(GenTree* treeNode)

for (unsigned i = 0; i < jumpCount; i++)
{
BasicBlock* target = *jumpTable++;
BasicBlock* target = (*jumpTable)->getDestinationBlock();
jumpTable++;
noway_assert(target->HasFlag(BBF_HAS_LABEL));

JITDUMP(" DD L_M%03u_" FMT_BB "\n", compiler->compMethodID, target->bbNum);
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6862,7 +6862,7 @@ class Compiler
bool optExtractInitTestIncr(
BasicBlock** pInitBlock, BasicBlock* bottom, BasicBlock* top, GenTree** ppInit, GenTree** ppTest, GenTree** ppIncr);

void optRedirectBlock(BasicBlock* blk,
void optInitDuplicatedBlockTargets(BasicBlock* blk,
Copy link
Member

Choose a reason for hiding this comment

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

Not sold on the name -- optSetMappedBlockTargets?

But we can sort it out later.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jakobbotsch how do you feel about optSetMappedBlockTargets?

Copy link
Member

Choose a reason for hiding this comment

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

I'd be ok with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll fix it now then

BasicBlock* newBlk,
BlockToBlockMap* redirectMap);

Expand Down
96 changes: 37 additions & 59 deletions src/coreclr/jit/fgbasic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,36 +382,26 @@ void Compiler::fgChangeSwitchBlock(BasicBlock* oldSwitchBlock, BasicBlock* newSw
assert(fgPredsComputed);

// Walk the switch's jump table, updating the predecessor for each branch.
for (BasicBlock* const bJump : oldSwitchBlock->SwitchTargets())
{
noway_assert(bJump != nullptr);

// Note that if there are duplicate branch targets in the switch jump table,
// fgRemoveRefPred()/fgAddRefPred() will do the right thing: the second and
// subsequent duplicates will simply subtract from and add to the duplicate
// count (respectively).
//
// However this does the "wrong" thing with respect to edge profile
// data; the old edge is not returned by fgRemoveRefPred until it has
// a dup count of 0, and the fgAddRefPred only uses the optional
// old edge arg when the new edge is first created.
//
// Remove the old edge [oldSwitchBlock => bJump]
//
assert(bJump->countOfInEdges() > 0);
FlowEdge* const oldEdge = fgRemoveRefPred(bJump, oldSwitchBlock);
BBswtDesc* swtDesc = oldSwitchBlock->GetSwitchTargets();

//
// Create the new edge [newSwitchBlock => bJump]
//
FlowEdge* const newEdge = fgAddRefPred(bJump, newSwitchBlock);
for (unsigned i = 0; i < swtDesc->bbsCount; i++)
{
FlowEdge* succEdge = swtDesc->bbsDstTab[i];
assert(succEdge != nullptr);

// Handle the profile update, once we get our hands on the old edge.
//
if (oldEdge != nullptr)
if (succEdge->getSourceBlock() != oldSwitchBlock)
{
assert(!newEdge->hasLikelihood());
newEdge->setLikelihood(oldEdge->getLikelihood());
// swtDesc can have duplicate targets, so we may have updated this edge already
//
assert(succEdge->getSourceBlock() == newSwitchBlock);
assert(succEdge->getDupCount() > 1);
}
else
{
// Redirect edge's source block from oldSwitchBlock to newSwitchBlock,
// and keep successor block's pred list in order
//
fgReplacePred(succEdge, newSwitchBlock);
}
}

Expand Down Expand Up @@ -709,29 +699,17 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas

case BBJ_SWITCH:
{
unsigned const jumpCnt = block->GetSwitchTargets()->bbsCount;
BasicBlock** const jumpTab = block->GetSwitchTargets()->bbsDstTab;
bool changed = false;
unsigned const jumpCnt = block->GetSwitchTargets()->bbsCount;
FlowEdge** const jumpTab = block->GetSwitchTargets()->bbsDstTab;
bool changed = false;

for (unsigned i = 0; i < jumpCnt; i++)
{
if (jumpTab[i] == oldTarget)
if (jumpTab[i]->getDestinationBlock() == oldTarget)
{
jumpTab[i] = newTarget;
changed = true;
FlowEdge* const oldEdge = fgRemoveRefPred(oldTarget, block);
FlowEdge* const newEdge = fgAddRefPred(newTarget, block, oldEdge);

// Handle the profile update, once we get our hands on the old edge.
// (see notes in fgChangeSwitchBlock for why this extra step is necessary)
//
// We do it slightly differently here so we don't lose the old
// edge weight propagation that would sometimes happen
//
if ((oldEdge != nullptr) && !newEdge->hasLikelihood())
{
newEdge->setLikelihood(oldEdge->getLikelihood());
}
fgRemoveRefPred(jumpTab[i]);
jumpTab[i] = fgAddRefPred(newTarget, block, jumpTab[i]);
changed = true;
}
}

Expand Down Expand Up @@ -3041,23 +3019,23 @@ void Compiler::fgLinkBasicBlocks()

case BBJ_SWITCH:
{
unsigned jumpCnt = curBBdesc->GetSwitchTargets()->bbsCount;
BasicBlock** jumpPtr = curBBdesc->GetSwitchTargets()->bbsDstTab;
unsigned jumpCnt = curBBdesc->GetSwitchTargets()->bbsCount;
FlowEdge** jumpPtr = curBBdesc->GetSwitchTargets()->bbsDstTab;

do
{
BasicBlock* jumpDest = fgLookupBB((unsigned)*(size_t*)jumpPtr);
*jumpPtr = jumpDest;
fgAddRefPred<initializingPreds>(jumpDest, curBBdesc);
if ((*jumpPtr)->bbNum <= curBBdesc->bbNum)
BasicBlock* jumpDest = fgLookupBB((unsigned)*(size_t*)jumpPtr);
FlowEdge* const newEdge = fgAddRefPred<initializingPreds>(jumpDest, curBBdesc);
*jumpPtr = newEdge;
if (jumpDest->bbNum <= curBBdesc->bbNum)
{
fgMarkBackwardJump(*jumpPtr, curBBdesc);
fgMarkBackwardJump(jumpDest, curBBdesc);
}
} while (++jumpPtr, --jumpCnt);

/* Default case of CEE_SWITCH (next block), is at end of jumpTab[] */

noway_assert(curBBdesc->NextIs(*(jumpPtr - 1)));
noway_assert(curBBdesc->NextIs((*(jumpPtr - 1))->getDestinationBlock()));
break;
}

Expand Down Expand Up @@ -3220,8 +3198,8 @@ unsigned Compiler::fgMakeBasicBlocks(const BYTE* codeAddr, IL_OFFSET codeSize, F
unsigned jmpBase;
unsigned jmpCnt; // # of switch cases (excluding default)

BasicBlock** jmpTab;
BasicBlock** jmpPtr;
FlowEdge** jmpTab;
FlowEdge** jmpPtr;

/* Allocate the switch descriptor */

Expand All @@ -3238,7 +3216,7 @@ unsigned Compiler::fgMakeBasicBlocks(const BYTE* codeAddr, IL_OFFSET codeSize, F

/* Allocate the jump table */

jmpPtr = jmpTab = new (this, CMK_BasicBlock) BasicBlock*[jmpCnt + 1];
jmpPtr = jmpTab = new (this, CMK_FlowEdge) FlowEdge*[jmpCnt + 1];

/* Fill in the jump table */

Expand All @@ -3248,12 +3226,12 @@ unsigned Compiler::fgMakeBasicBlocks(const BYTE* codeAddr, IL_OFFSET codeSize, F
codeAddr += 4;

// store the offset in the pointer. We change these in fgLinkBasicBlocks().
*jmpPtr++ = (BasicBlock*)(size_t)(jmpBase + jmpDist);
*jmpPtr++ = (FlowEdge*)(size_t)(jmpBase + jmpDist);
}

/* Append the default label to the target table */

*jmpPtr++ = (BasicBlock*)(size_t)jmpBase;
*jmpPtr++ = (FlowEdge*)(size_t)jmpBase;

/* Make sure we found the right number of labels */

Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/jit/fgdiagnostic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1101,7 +1101,7 @@ bool Compiler::fgDumpFlowGraph(Phases phase, PhasePosition pos)
{
fprintf(fgxFile, "\n switchCases=\"%d\"", edge->getDupCount());
}
if (bSource->GetSwitchTargets()->getDefault() == bTarget)
if (bSource->GetSwitchTargets()->getDefault()->getDestinationBlock() == bTarget)
{
fprintf(fgxFile, "\n switchDefault=\"true\"");
}
Expand Down Expand Up @@ -2066,12 +2066,12 @@ void Compiler::fgTableDispBasicBlock(const BasicBlock* block,

const BBswtDesc* const jumpSwt = block->GetSwitchTargets();
const unsigned jumpCnt = jumpSwt->bbsCount;
BasicBlock** const jumpTab = jumpSwt->bbsDstTab;
FlowEdge** const jumpTab = jumpSwt->bbsDstTab;

for (unsigned i = 0; i < jumpCnt; i++)
{
printedBlockWidth += 1 /* space/comma */;
printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(jumpTab[i]));
printf("%c%s", (i == 0) ? ' ' : ',', dspBlockNum(jumpTab[i]->getDestinationBlock()));

const bool isDefault = jumpSwt->bbsHasDefault && (i == jumpCnt - 1);
if (isDefault)
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/fgehopt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1099,7 +1099,7 @@ PhaseStatus Compiler::fgCloneFinally()
}
else
{
optRedirectBlock(block, newBlock, &blockMap);
optInitDuplicatedBlockTargets(block, newBlock, &blockMap);
}
}

Expand Down
Loading