Skip to content

Commit

Permalink
[WIP] Remove the BBJ_ALWAYS block of BBJ_CALLFINALLY/BBJ_ALWAYS pairs
Browse files Browse the repository at this point in the history 
This is a partial implementation of the original proposal in
dotnet#95355

It has run up against difficult-to-solve issues, and has been abandoned.
  • Loading branch information
@BruceForstall
BruceForstall committed Dec 10, 2023
1 parent 425cfb9 commit 84930b4
Show file tree
Hide file tree
Showing 25 changed files with 452 additions and 936 deletions.
105 changes: 41 additions & 64 deletions src/coreclr/jit/block.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,7 @@ FlowEdge* Compiler::BlockPredsWithEH(BasicBlock* blk)

res = blk->bbPreds;
unsigned tryIndex = blk->getHndIndex();
// Add all blocks handled by this handler (except for second blocks of BBJ_CALLFINALLY/BBJ_ALWAYS pairs;
// these cannot cause transfer to the handler...)
// Add all blocks handled by this handler.
// TODO-Throughput: It would be nice if we could iterate just over the blocks in the try, via
// something like:
// for (BasicBlock* bb = ehblk->ebdTryBeg; bb != ehblk->ebdTryLast->Next(); bb = bb->Next())
Expand All @@ -140,7 +139,7 @@ FlowEdge* Compiler::BlockPredsWithEH(BasicBlock* blk)
// more than one sequence of contiguous blocks. We need to find a better way to do this.
for (BasicBlock* const bb : Blocks())
{
if (bbInExnFlowRegions(tryIndex, bb) && !bb->isBBCallAlwaysPairTail())
if (bbInExnFlowRegions(tryIndex, bb))
{
res = new (this, CMK_FlowEdge) FlowEdge(bb, res);

Expand Down Expand Up @@ -547,10 +546,6 @@ void BasicBlock::dspFlags()
{
printf("pc-ppoint ");
}
if (HasFlag(BBF_RETLESS_CALL))
{
printf("retless ");
}
if (HasFlag(BBF_LOOP_PREHEADER))
{
printf("LoopPH ");
Expand Down Expand Up @@ -742,7 +737,14 @@ void BasicBlock::dspJumpKind()
break;

case BBJ_CALLFINALLY:
printf(" -> " FMT_BB " (callf)", bbJumpDest->bbNum);
if (bbFinallyContinuation == nullptr)
{
printf(" -> " FMT_BB " C-> * (callf)", bbJumpDest->bbNum);
}
else
{
printf(" -> " FMT_BB " C-> " FMT_BB " (callf)", bbJumpDest->bbNum, bbFinallyContinuation->bbNum);
}
break;

case BBJ_COND:
Expand Down Expand Up @@ -886,6 +888,36 @@ bool BasicBlock::CloneBlockState(
return true;
}


//------------------------------------------------------------------------
// CopyTarget: Copy the block kind and targets.
//
// Arguments:
// compiler - Jit compiler instance
// from - Block to copy from
//
void BasicBlock::CopyTarget(Compiler* compiler, const BasicBlock* from)
{
switch (from->GetJumpKind())
{
case BBJ_SWITCH:
SetSwitchKindAndTarget(new (compiler, CMK_BasicBlock) BBswtDesc(compiler, from->GetJumpSwt()));
break;
case BBJ_EHFINALLYRET:
SetJumpKindAndTarget(BBJ_EHFINALLYRET, new (compiler, CMK_BasicBlock) BBehfDesc(compiler, from->GetJumpEhf()));
break;
case BBJ_CALLFINALLY:
SetJumpKindAndTarget(from->GetJumpKind(), from->GetJumpDest());
SetFinallyContinuation(from->GetFinallyContinuation());
break;
default:
SetJumpKindAndTarget(from->GetJumpKind(), from->GetJumpDest());
CopyFlags(from, BBF_NONE_QUIRK);
break;
}
assert(KindIs(from->GetJumpKind()));
}

// LIR helpers
void BasicBlock::MakeLIR(GenTree* firstNode, GenTree* lastNode)
{
Expand Down Expand Up @@ -1109,14 +1141,12 @@ bool BasicBlock::bbFallsThrough() const
case BBJ_ALWAYS:
case BBJ_LEAVE:
case BBJ_SWITCH:
case BBJ_CALLFINALLY:
return false;

case BBJ_COND:
return true;

case BBJ_CALLFINALLY:
return !HasFlag(BBF_RETLESS_CALL);

default:
assert(!"Unknown bbJumpKind in bbFallsThrough()");
return true;
Expand Down Expand Up @@ -1625,59 +1655,6 @@ BasicBlock* BasicBlock::New(Compiler* compiler, BBjumpKinds jumpKind, unsigned j
return block;
}

//------------------------------------------------------------------------
// isBBCallAlwaysPair: Determine if this is the first block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair
//
// Return Value:
// True iff "this" is the first block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair
// -- a block corresponding to an exit from the try of a try/finally.
//
// Notes:
// In the flow graph, this becomes a block that calls the finally, and a second, immediately
// following empty block (in the bbNext chain) to which the finally will return, and which
// branches unconditionally to the next block to be executed outside the try/finally.
// Note that code is often generated differently than this description. For example, on ARM,
// the target of the BBJ_ALWAYS is loaded in LR (the return register), and a direct jump is
// made to the 'finally'. The effect is that the 'finally' returns directly to the target of
// the BBJ_ALWAYS. A "retless" BBJ_CALLFINALLY is one that has no corresponding BBJ_ALWAYS.
// This can happen if the finally is known to not return (e.g., it contains a 'throw'). In
// that case, the BBJ_CALLFINALLY flags has BBF_RETLESS_CALL set. Note that ARM never has
// "retless" BBJ_CALLFINALLY blocks due to a requirement to use the BBJ_ALWAYS for
// generating code.
//
bool BasicBlock::isBBCallAlwaysPair() const
{
if (this->KindIs(BBJ_CALLFINALLY) && !this->HasFlag(BBF_RETLESS_CALL))
{
// Some asserts that the next block is a BBJ_ALWAYS of the proper form.
assert(!this->IsLast());
assert(this->Next()->KindIs(BBJ_ALWAYS));
assert(this->Next()->HasFlag(BBF_KEEP_BBJ_ALWAYS));
assert(this->Next()->isEmpty());

return true;
}
else
{
return false;
}
}

//------------------------------------------------------------------------
// isBBCallAlwaysPairTail: Determine if this is the last block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair
//
// Return Value:
// True iff "this" is the last block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair
// -- a block corresponding to an exit from the try of a try/finally.
//
// Notes:
// See notes on isBBCallAlwaysPair(), above.
//
bool BasicBlock::isBBCallAlwaysPairTail() const
{
return (bbPrev != nullptr) && bbPrev->isBBCallAlwaysPair();
}

//------------------------------------------------------------------------
// hasEHBoundaryIn: Determine if this block begins at an EH boundary.
//
Expand Down
38 changes: 27 additions & 11 deletions src/coreclr/jit/block.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,11 +405,10 @@ enum BasicBlockFlags : unsigned __int64
BBF_HAS_MDARRAYREF = MAKE_BBFLAG(22), // Block has a multi-dimensional array reference
BBF_HAS_NEWOBJ = MAKE_BBFLAG(23), // BB contains 'new' of an object type.

BBF_RETLESS_CALL = MAKE_BBFLAG(24), // BBJ_CALLFINALLY that will never return (and therefore, won't need a paired
// BBJ_ALWAYS); see isBBCallAlwaysPair().
BBF_LOOP_PREHEADER = MAKE_BBFLAG(25), // BB is a loop preheader block
BBF_COLD = MAKE_BBFLAG(26), // BB is cold
BBF_PROF_WEIGHT = MAKE_BBFLAG(27), // BB weight is computed from profile data
// TODO-CALLFINALLY: remove BBF_KEEP_BBJ_ALWAYS?
BBF_KEEP_BBJ_ALWAYS = MAKE_BBFLAG(28), // A special BBJ_ALWAYS block, used by EH code generation. Keep the jump kind
// as BBJ_ALWAYS. Used for the paired BBJ_ALWAYS block following the
// BBJ_CALLFINALLY block, as well as, on x86, the final step block out of a
Expand Down Expand Up @@ -444,7 +443,7 @@ enum BasicBlockFlags : unsigned __int64

// Flags a block should not have had before it is split.

BBF_SPLIT_NONEXIST = BBF_LOOP_HEAD | BBF_RETLESS_CALL | BBF_LOOP_PREHEADER | BBF_COLD,
BBF_SPLIT_NONEXIST = BBF_LOOP_HEAD | BBF_LOOP_PREHEADER | BBF_COLD,

// Flags lost by the top block when a block is split.
// Note, this is a conservative guess.
Expand Down Expand Up @@ -532,6 +531,8 @@ struct BasicBlock : private LIR::Range
BBehfDesc* bbJumpEhf; // BBJ_EHFINALLYRET descriptor
};

BasicBlock* bbFinallyContinuation; // Only used by BBJ_CALLFINALLY. Where the called `finally` should return.

public:
static BasicBlock* New(Compiler* compiler);
static BasicBlock* New(Compiler* compiler, BBjumpKinds jumpKind, BasicBlock* jumpDest = nullptr);
Expand Down Expand Up @@ -700,6 +701,26 @@ struct BasicBlock : private LIR::Range
bbJumpEhf = jumpEhf;
}

// Which block types can have a non-null `bbFinallyContinuation`.
bool HasFinallyContinuation() const
{
return KindIs(BBJ_CALLFINALLY);
}

// Note that a finally continuation can be null for "retless" callfinally (the called finally
// is known to never return due to an unconditional `throw`).
BasicBlock* GetFinallyContinuation() const
{
assert(HasFinallyContinuation());
return bbFinallyContinuation;
}

void SetFinallyContinuation(BasicBlock* finallyContinuation)
{
assert(HasFinallyContinuation());
bbFinallyContinuation = finallyContinuation;
}

private:
BasicBlockFlags bbFlags;

Expand Down Expand Up @@ -943,14 +964,6 @@ struct BasicBlock : private LIR::Range

bool isValid() const;

// Returns "true" iff "this" is the first block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair --
// a block corresponding to an exit from the try of a try/finally.
bool isBBCallAlwaysPair() const;

// Returns "true" iff "this" is the last block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair --
// a block corresponding to an exit from the try of a try/finally.
bool isBBCallAlwaysPairTail() const;

bool KindIs(BBjumpKinds kind) const
{
return bbJumpKind == kind;
Expand Down Expand Up @@ -1579,6 +1592,9 @@ struct BasicBlock : private LIR::Range
static bool CloneBlockState(
Compiler* compiler, BasicBlock* to, const BasicBlock* from, unsigned varNum = (unsigned)-1, int varVal = 0);

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

void MakeLIR(GenTree* firstNode, GenTree* lastNode);
bool IsLIR() const;

Expand Down
25 changes: 5 additions & 20 deletions src/coreclr/jit/codegenarm.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,52 +119,37 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block)
{
GetEmitter()->emitIns_J(INS_bl, block->GetJumpDest());

BasicBlock* nextBlock = block->Next();
BasicBlock* finallyContinuation = block->GetFinallyContinuation();

if (block->HasFlag(BBF_RETLESS_CALL))
if (finallyContinuation == nullptr)
{
BasicBlock* nextBlock = block->Next();
if ((nextBlock == nullptr) || !BasicBlock::sameEHRegion(block, nextBlock))
{
instGen(INS_BREAKPOINT);
}
}
else
{
assert((nextBlock != nullptr) && nextBlock->isBBCallAlwaysPairTail());

// Because of the way the flowgraph is connected, the liveness info for this one instruction
// after the call is not (can not be) correct in cases where a variable has a last use in the
// handler. So turn off GC reporting for this single instruction.
GetEmitter()->emitDisableGC();

BasicBlock* const jumpDest = nextBlock->GetJumpDest();

// Now go to where the finally funclet needs to return to.
if (nextBlock->NextIs(jumpDest) && !compiler->fgInDifferentRegions(nextBlock, jumpDest))
if (block->NextIs(finallyContinuation) && !compiler->fgInDifferentRegions(block, finallyContinuation))
{
// Fall-through.
// TODO-ARM-CQ: Can we get rid of this instruction, and just have the call return directly
// to the next instruction? This would depend on stack walking from within the finally
// handler working without this instruction being in this special EH region.
instGen(INS_nop);
}
else
{
GetEmitter()->emitIns_J(INS_b, jumpDest);
GetEmitter()->emitIns_J(INS_b, finallyContinuation);
}

GetEmitter()->emitEnableGC();
}

// The BBJ_ALWAYS is used because the BBJ_CALLFINALLY can't point to the
// jump target using bbJumpDest - that is already used to point
// to the finally block. So just skip past the BBJ_ALWAYS unless the
// block is RETLESS.
if (!block->HasFlag(BBF_RETLESS_CALL))
{
assert(block->isBBCallAlwaysPair());
block = nextBlock;
}
return block;
}

Expand Down
18 changes: 4 additions & 14 deletions src/coreclr/jit/codegenarm64.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2161,8 +2161,9 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block)
GetEmitter()->emitIns_J(INS_bl_local, block->GetJumpDest());

BasicBlock* const nextBlock = block->Next();
BasicBlock* const blockContinuation = block->GetFinallyContinuation();

if (block->HasFlag(BBF_RETLESS_CALL))
if (blockContinuation == nullptr)
{
// We have a retless call, and the last instruction generated was a call.
// If the next block is in a different EH region (or is the end of the code
Expand All @@ -2181,10 +2182,8 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block)
// handler. So turn off GC reporting for this single instruction.
GetEmitter()->emitDisableGC();

BasicBlock* const jumpDest = nextBlock->GetJumpDest();

// Now go to where the finally funclet needs to return to.
if (nextBlock->NextIs(jumpDest) && !compiler->fgInDifferentRegions(nextBlock, jumpDest))
if (nextBlock->NextIs(blockContinuation) && !compiler->fgInDifferentRegions(nextBlock, blockContinuation))
{
// Fall-through.
// TODO-ARM64-CQ: Can we get rid of this instruction, and just have the call return directly
Expand All @@ -2194,21 +2193,12 @@ BasicBlock* CodeGen::genCallFinally(BasicBlock* block)
}
else
{
inst_JMP(EJ_jmp, jumpDest);
inst_JMP(EJ_jmp, blockContinuation);
}

GetEmitter()->emitEnableGC();
}

// The BBJ_ALWAYS is used because the BBJ_CALLFINALLY can't point to the
// jump target using bbJumpDest - that is already used to point
// to the finally block. So just skip past the BBJ_ALWAYS unless the
// block is RETLESS.
if (!block->HasFlag(BBF_RETLESS_CALL))
{
assert(block->isBBCallAlwaysPair());
block = nextBlock;
}
return block;
}

Expand Down
Loading

0 comments on commit 84930b4

Please sign in to comment.