Skip to content

Commit

Permalink
Fix insertion of alignment after BBJ_CALLFINALLY/BBJ_ALWAYS
Browse files Browse the repository at this point in the history
The Runtime_76346 test exposed a case where, in the case of STRESS_EMITTER,
we were inserting breakpoint instructions instead of NOPs for loop alignment
when the alignment followed an unconditional branch. However, it wasn't
considering the case of a BBJ_CALLFINALLY/BBJ_ALWAYS pair immediately
followed by a loop head.

This pointed out that alignment instructions should never be inserted in that
BBJ_ALWAYS block, since that block shouldn't contain any code. Inserting the
alignment NOPs affected the reported range of the EH cloned finally region
in the FEATURE_EH_CALLFINALLY_THUNKS case.

In these special cases, we simply give up on trying to align the loop.

Fixes dotnet#76910
  • Loading branch information
BruceForstall committed Oct 13, 2022
1 parent 3435b39 commit 1f0043d
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 4 deletions.
5 changes: 5 additions & 0 deletions src/coreclr/jit/codegenlinear.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,12 @@ void CodeGen::genCodeForBBlist()
// compJitAlignLoopBoundary.
// For adaptive alignment, alignment instruction will always be of 15 bytes for xarch
// and 16 bytes for arm64.

assert(ShouldAlignLoops());
assert(!block->isBBCallAlwaysPairTail());
#if FEATURE_EH_CALLFINALLY_THUNKS
assert(block->bbJumpKind != BBJ_CALLFINALLY);
#endif // FEATURE_EH_CALLFINALLY_THUNKS

GetEmitter()->emitLoopAlignment(DEBUG_ARG1(block->bbJumpKind == BBJ_ALWAYS));
}
Expand Down
38 changes: 34 additions & 4 deletions src/coreclr/jit/compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5090,14 +5090,44 @@ PhaseStatus Compiler::placeLoopAlignInstructions()
BasicBlock* const loopTop = block->bbNext;

// If jmp was not found, then block before the loop start is where align instruction will be added.
// There are two special cases:
// 1. If the block before the loop start is a retless BBJ_CALLFINALLY with
// FEATURE_EH_CALLFINALLY_THUNKS, we can't add alignment because it will affect reported EH
// region range.
// 2. If the previous block is the BBJ_ALWAYS of a BBJ_CALLFINALLY/BBJ_ALWAYS pair, then we
// can't add alignment because we can't add instructions in that block. In the
// FEATURE_EH_CALLFINALLY_THUNKS case, it would affect the reported EH, as above.
//
// Currently, we don't align loops for these cases.
//
if (bbHavingAlign == nullptr)
{
// In some odd cases we may see blocks within the loop before we see the
// top block of the loop. Just bail on aligning such loops.
//
if ((block->bbNatLoopNum != BasicBlock::NOT_IN_LOOP) && (block->bbNatLoopNum == loopTop->bbNatLoopNum))
bool isSpecialCallFinally = block->isBBCallAlwaysPairTail();
#if FEATURE_EH_CALLFINALLY_THUNKS
if (block->bbJumpKind == BBJ_CALLFINALLY)
{
// It must be a retless BBJ_CALLFINALLY if we get here.
assert(!block->isBBCallAlwaysPair());

// In the case of FEATURE_EH_CALLFINALLY_THUNKS, we can't put the align instruction in a retless
// BBJ_CALLFINALLY either, because it alters the "cloned finally" region reported to the VM.
// In the x86 case (the only !FEATURE_EH_CALLFINALLY_THUNKS that supports retless
// BBJ_CALLFINALLY), we allow it.
isSpecialCallFinally = true;
}
#endif // FEATURE_EH_CALLFINALLY_THUNKS

if (isSpecialCallFinally)
{
loopTop->unmarkLoopAlign(this DEBUG_ARG("block before loop is special callfinally/always block"));
madeChanges = true;
}
else if ((block->bbNatLoopNum != BasicBlock::NOT_IN_LOOP) &&
(block->bbNatLoopNum == loopTop->bbNatLoopNum))
{
// In some odd cases we may see blocks within the loop before we see the
// top block of the loop. Just bail on aligning such loops.
//
loopTop->unmarkLoopAlign(this DEBUG_ARG("loop block appears before top of loop"));
madeChanges = true;
}
Expand Down

0 comments on commit 1f0043d

Please sign in to comment.