Skip to content

JIT: Refactor Compiler::optRedirectBlock; remove BasicBlock::CopyTarget #98526

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
amanasifkhalid merged 2 commits into from
Feb 16, 2024
Merged

JIT: Refactor Compiler::optRedirectBlock; remove BasicBlock::CopyTarget #98526

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
c6c7d2a
Refactor optRedirectBlock
amanasifkhalid Feb 15, 2024
b11f5f2
Remove BasicBlock::CopyTarget
amanasifkhalid Feb 15, 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
56 changes: 16 additions & 40 deletions src/coreclr/jit/block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,46 +837,6 @@ void BasicBlock::CloneBlockState(Compiler* compiler, BasicBlock* to, const Basic
}
}

//------------------------------------------------------------------------
// CopyTarget: Copy the block kind and targets. The targets in the `from` block remain valid.
// Use `TransferTarget` to copy the pointer to the target descriptor (e.g., for BBJ_SWITCH/BBJ_EHFINALLYRET)
// after which the `from` block target is invalid.
//
// Arguments:
// compiler - Jit compiler instance
// from - Block to copy from
//
void BasicBlock::CopyTarget(Compiler* compiler, const BasicBlock* from)
{
switch (from->GetKind())
{
case BBJ_SWITCH:
SetSwitch(new (compiler, CMK_BasicBlock) BBswtDesc(compiler, from->GetSwitchTargets()));
break;
case BBJ_EHFINALLYRET:
SetEhf(new (compiler, CMK_BasicBlock) BBehfDesc(compiler, from->GetEhfTargets()));
break;
case BBJ_COND:
SetCond(from->GetTrueTarget(), from->GetFalseTarget());
break;
case BBJ_ALWAYS:
SetKindAndTarget(from->GetKind(), from->GetTarget());
CopyFlags(from, BBF_NONE_QUIRK);
break;
case BBJ_CALLFINALLY:
case BBJ_CALLFINALLYRET:
case BBJ_EHCATCHRET:
case BBJ_EHFILTERRET:
case BBJ_LEAVE:
SetKindAndTarget(from->GetKind(), from->GetTarget());
break;
default:
SetKindAndTarget(from->GetKind()); // Clear the target
break;
}
assert(KindIs(from->GetKind()));
}

//------------------------------------------------------------------------
// TransferTarget: Like CopyTarget, but copies the target descriptors for block types which have
// them (BBJ_SWITCH/BBJ_EHFINALLYRET), that is, take their memory, after which the `from` block
Expand Down Expand Up @@ -1781,6 +1741,22 @@ bool BasicBlock::hasEHBoundaryOut() const
return returnVal;
}

//------------------------------------------------------------------------
// BBswtDesc copy ctor: copy a switch descriptor, but don't set up the jump table
//
// Arguments:
// other - existing switch descriptor to copy (except for its jump table)
//
BBswtDesc::BBswtDesc(const BBswtDesc* other)
: bbsDstTab(nullptr)
, bbsCount(other->bbsCount)
, bbsDominantCase(other->bbsDominantCase)
, bbsDominantFraction(other->bbsDominantFraction)
, bbsHasDefault(other->bbsHasDefault)
, bbsHasDominantCase(other->bbsHasDominantCase)
{
}

//------------------------------------------------------------------------
// BBswtDesc copy ctor: copy a switch descriptor
//
Expand Down
5 changes: 2 additions & 3 deletions src/coreclr/jit/block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1660,9 +1660,6 @@ struct BasicBlock : private LIR::Range
// Clone block state and statements from `from` block to `to` block (which must be new/empty)
static void CloneBlockState(Compiler* compiler, BasicBlock* to, const BasicBlock* from);

// Copy the block kind and targets. The `from` block is untouched.
void CopyTarget(Compiler* compiler, const BasicBlock* from);

// Copy the block kind and take memory ownership of the targets.
void TransferTarget(BasicBlock* from);

Expand Down Expand Up @@ -1839,6 +1836,8 @@ struct BBswtDesc
{
}

BBswtDesc(const BBswtDesc* other);

BBswtDesc(Compiler* comp, const BBswtDesc* other);

void removeDefault()
Expand Down
11 changes: 2 additions & 9 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6839,16 +6839,9 @@ class Compiler
bool optExtractInitTestIncr(
BasicBlock** pInitBlock, BasicBlock* bottom, BasicBlock* top, GenTree** ppInit, GenTree** ppTest, GenTree** ppIncr);

enum class RedirectBlockOption
{
DoNotChangePredLists, // do not modify pred lists
UpdatePredLists, // add/remove to pred lists
AddToPredLists, // only add to pred lists
};

void optRedirectBlock(BasicBlock* blk,
BlockToBlockMap* redirectMap,
const RedirectBlockOption = RedirectBlockOption::DoNotChangePredLists);
BasicBlock* newBlk,
BlockToBlockMap* redirectMap);
Comment on lines 6842 to +6844
Copy link
Member

@jakobbotsch jakobbotsch Feb 16, 2024

Choose a reason for hiding this comment

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

I'm not sure I understand the new function that well -- it requires me to have a block around that the previous function did not require me to have around, but what if I don't have that?

This function doesn't seem to be doing redirection anymore, it rather seems to be initializing a new block based on an old block and a map. Which is fine if that's what we generally need, but the old function seemed much more general than this -- basically a generalization of fgReplaceJumpTarget to replace multiple targets at once. How would we implement that functionality with the new scheme? It is conceivable to me that we are going to need it again.

Copy link
Contributor Author

@amanasifkhalid amanasifkhalid Feb 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

it requires me to have a block around that the previous function did not require me to have around, but what if I don't have that?

For the current callers of optRedirectBlock, newBlk is obtained from redirectMap -- for each copied block blk, we expect there to be a blk -> newBlk mapping. I could refactor optRedirectBlock slightly so that it only takes blk, and retrieves newBlk from redirectMap. We'd always expect this mapping to exist, right? Can you foresee a situation where we wouldn't have that?

This function doesn't seem to be doing redirection anymore, it rather seems to be initializing a new block based on an old block and a map. Which is fine if that's what we generally need, but the old function seemed much more general than this

In my opinion, the old version was trying to do a few too many things, making the semantics of its callers a bit awkward. Previously, callers would have to copy the old block's target(s) to the new block with BasicBlock::CopyTarget without setting up any pred edges, and then tell optRedirectBlock to add pred edges, while deciding what the real target should be. I don't think this approach will hold up well once we've replaced block targets with successor edges: Once we've done that, it won't be possible for a block to have a jump target without an edge to it, which is a state we'd previously establish before calling optRedirectBlock.

How would we implement that functionality with the new scheme? It is conceivable to me that we are going to need it again.

This would introduce some code duplication, but maybe we could have a version of optRedirectBlock that assumes newBlk's successors are initialized, and the current version (which we can rename to imply it is initializing successors, if you'd prefer) can continue to assume successors aren't initialized?

My next few PRs will be touching this method, so I'm happy to add any follow-up changes you recommend to those.

Copy link
Member

@jakobbotsch jakobbotsch Feb 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For the current callers of optRedirectBlock, newBlk is obtained from redirectMap -- for each copied block blk, we expect there to be a blk -> newBlk mapping. I could refactor optRedirectBlock slightly so that it only takes blk, and retrieves newBlk from redirectMap. We'd always expect this mapping to exist, right? Can you foresee a situation where we wouldn't have that?

I would not expect this mapping to always exist. As I mentioned above, I think optRedirectBlock should be viewed as a generalization of fgReplaceJumpTarget that allows replacing multiple targets in one operation. I.e. it was an operation similar to

for ((BasicBlock* key, BasicBlock* target) in blockMap) { fgReplaceJumpTarget(block, key, target); }

I agree the overloaded handling around preds before wasn't very pretty.

I think with the refactoring done in this PR the function should be renamed to something that indicates that it operates on a partially initialized block and is some form of initialization -- it no longer matches the above semantics. Maybe something like optInitializeDuplicatedBlockTargets?

I actually had local changes where I generalized optRedirectBlock to take a functor instead of a BlockToBlockMap, making it more easy to use for these kinds of more general block redirections. With that I think fgReplaceJumpTarget could be implemented in terms of optRedirectBlock (with a functor like [=](BasicBlock* target) { if (target == oldBlock) { return newBlock; } return target; }).

Copy link
Contributor Author

@amanasifkhalid amanasifkhalid Feb 19, 2024

Choose a reason for hiding this comment

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

I think with the refactoring done in this PR the function should be renamed to something that indicates that it operates on a partially initialized block and is some form of initialization -- it no longer matches the above semantics. Maybe something like optInitializeDuplicatedBlockTargets?

Sure thing, I can include that change in my next PR.

I actually had local changes where I generalized optRedirectBlock to take a functor instead of a BlockToBlockMap, making it more easy to use for these kinds of more general block redirections.

I like that idea. Are you ok with me wrapping up the successor edge refactor first, and then coming back to this? That should only take a few more PRs; I have the code ready locally.

Copy link
Member

@jakobbotsch jakobbotsch Feb 19, 2024

Choose a reason for hiding this comment

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

Sure -- also, I'm ok with leaving the function out entirely and leaving it as it is right now (with the rename). We can add the more general version when we find a need for it.


// Marks the containsCall information to "loop" and any parent loops.
void AddContainsCallAllContainingLoops(FlowGraphNaturalLoop* loop);
Expand Down
22 changes: 11 additions & 11 deletions src/coreclr/jit/fgbasic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,14 +620,13 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas
case BBJ_EHCATCHRET:
case BBJ_EHFILTERRET:
case BBJ_LEAVE: // This function can be called before import, so we still have BBJ_LEAVE

if (block->TargetIs(oldTarget))
{
block->SetTarget(newTarget);
FlowEdge* const oldEdge = fgRemoveRefPred(oldTarget, block);
fgAddRefPred(newTarget, block, oldEdge);
}
{
assert(block->TargetIs(oldTarget));
block->SetTarget(newTarget);
FlowEdge* const oldEdge = fgRemoveRefPred(oldTarget, block);
fgAddRefPred(newTarget, block, oldEdge);
break;
}

case BBJ_COND:

Expand Down Expand Up @@ -709,6 +708,10 @@ void Compiler::fgReplaceJumpTarget(BasicBlock* block, BasicBlock* oldTarget, Bas
break;
}

case BBJ_EHFINALLYRET:
fgReplaceEhfSuccessor(block, oldTarget, newTarget);
break;

default:
assert(!"Block doesn't have a jump target!");
unreached();
Expand Down Expand Up @@ -5297,11 +5300,8 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable)
case BBJ_ALWAYS:
case BBJ_EHCATCHRET:
case BBJ_SWITCH:
fgReplaceJumpTarget(predBlock, block, succBlock);
break;

case BBJ_EHFINALLYRET:
fgReplaceEhfSuccessor(predBlock, block, succBlock);
fgReplaceJumpTarget(predBlock, block, succBlock);
break;
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/fgehopt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1099,8 +1099,7 @@ PhaseStatus Compiler::fgCloneFinally()
}
else
{
newBlock->CopyTarget(this, block);
optRedirectBlock(newBlock, &blockMap, RedirectBlockOption::AddToPredLists);
optRedirectBlock(block, newBlock, &blockMap);
}
}

Expand Down
33 changes: 3 additions & 30 deletions src/coreclr/jit/flowgraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5608,36 +5608,9 @@ void FlowGraphNaturalLoop::Duplicate(BasicBlock** insertAfter, BlockToBlockMap*
// Jump target should not be set yet
assert(!newBlk->HasInitializedTarget());

// First copy the jump destination(s) from "blk".
newBlk->CopyTarget(comp, blk);

// Now redirect the new block according to "blockMap".
comp->optRedirectBlock(newBlk, map);

// Add predecessor edges for the new successors, as well as the fall-through paths.
switch (newBlk->GetKind())
{
case BBJ_ALWAYS:
case BBJ_CALLFINALLY:
case BBJ_CALLFINALLYRET:
comp->fgAddRefPred(newBlk->GetTarget(), newBlk);
break;

case BBJ_COND:
comp->fgAddRefPred(newBlk->GetFalseTarget(), newBlk);
comp->fgAddRefPred(newBlk->GetTrueTarget(), newBlk);
break;

case BBJ_SWITCH:
for (BasicBlock* const switchDest : newBlk->SwitchTargets())
{
comp->fgAddRefPred(switchDest, newBlk);
}
break;

default:
break;
}
// Redirect the new block according to "blockMap".
// opRedirectBlock will set newBlk's successors, and add pred edges for the successors.
comp->optRedirectBlock(blk, newBlk, map);

return BasicBlockVisit::Continue;
});
Expand Down
Loading