Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Commit

Permalink
Improve const return block placement
Browse files Browse the repository at this point in the history 
Tweak a few things so that generated constant return blocks get laid out
more optimally:

 - Don't set BBF_DONT_REMOVE on them; it's ok to move them around, and
   if all references to them get dropped, it's fine to eliminate them.
 - Insert them immediately after their lexically-last predecessor when
   generating them; this increases the likelihood of using fallthrough
   rather than gotos to target them in the face of fgReorderBlocks'
   reticence to reorder code that we don't have IBC data for and that
   hasn't been marked rare.
 - Make fgReorderBlocks slightly more aggressive for a pattern that now
   shows up somewhat routinely for returning compound conditionals from
   predicate functions.
  • Loading branch information
@JosephTremoulet
JosephTremoulet committed Sep 11, 2017
1 parent eb2e9f9 commit 20ac1a9
Show file tree
Hide file tree
Showing 2 changed files with 158 additions and 4 deletions.
80 changes: 76 additions & 4 deletions src/jit/flowgraph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8187,6 +8187,10 @@ class MergedReturns
// merged constant return blocks.
ssize_t returnConstants[ReturnCountHardLimit];

// Indicators of where in the lexical block list we'd like to place
// each constant return block.
BasicBlock* insertionPoints[ReturnCountHardLimit];

// Number of return blocks allowed
PhasedVar<unsigned> maxReturns;

Expand Down Expand Up @@ -8271,6 +8275,49 @@ class MergedReturns
return Merge(nullptr, 0);
}

//------------------------------------------------------------------------
// PlaceReturns: Move any generated const return blocks to an appropriate
// spot in the lexical block list.
//
// Notes:
// The goal is to set things up favorably for a reasonable layout without
// putting too much burden on fgReorderBlocks; in particular, since that
// method doesn't (currently) shuffle non-profile, non-rare code to create
// fall-through and reduce gotos, this method places each const return
// block immediately after its last predecessor, so that the flow from
// there to it can become fallthrough without requiring any motion to be
// performed by fgReorderBlocks.
//
void PlaceReturns()
{
if (!mergingReturns)
{
// No returns generated => no returns to place.
return;
}

for (unsigned index = 0; index < comp->fgReturnCount; ++index)
{
BasicBlock* returnBlock = returnBlocks[index];
BasicBlock* genReturnBlock = comp->genReturnBB;
if (returnBlock == genReturnBlock)
{
continue;
}

BasicBlock* insertionPoint = insertionPoints[index];
assert(insertionPoint != nullptr);

comp->fgUnlinkBlock(returnBlock);
comp->fgMoveBlocksAfter(returnBlock, returnBlock, insertionPoint);
// Treat the merged return block as belonging to the same EH region
// as the insertion point block, to make sure we don't break up
// EH regions; since returning a constant won't throw, this won't
// affect program behavior.
comp->fgExtendEHRegionAfter(insertionPoint);
}
}

private:
//------------------------------------------------------------------------
// CreateReturnBB: Create a basic block to serve as a merged return point, stored to
Expand All @@ -8293,7 +8340,7 @@ class MergedReturns
newReturnBB->bbRefs = 1; // bbRefs gets update later, for now it should be 1
comp->fgReturnCount++;

newReturnBB->bbFlags |= (BBF_INTERNAL | BBF_DONT_REMOVE);
newReturnBB->bbFlags |= BBF_INTERNAL;

noway_assert(newReturnBB->bbNext == nullptr);

Expand Down Expand Up @@ -8441,7 +8488,8 @@ class MergedReturns
{
// We have a constant. Now find or create a corresponding return block.

BasicBlock* constReturnBlock = FindConstReturnBlock(retConst, searchLimit);
unsigned index;
BasicBlock* constReturnBlock = FindConstReturnBlock(retConst, searchLimit, &index);

if (constReturnBlock == nullptr)
{
Expand Down Expand Up @@ -8478,6 +8526,13 @@ class MergedReturns
assert(returnBlock->lastStmt()->gtStmtExpr->OperIs(GT_RETURN));
assert(returnBlock->lastStmt()->gtStmtExpr->gtGetOp1()->IsIntegralConst());
comp->fgRemoveStmt(returnBlock, returnBlock->lastStmt());

// Using 'returnBlock' as the insertion point for 'mergedReturnBlock'
// will give it a chance to use fallthrough rather than BBJ_ALWAYS.
// Resetting this after each merge ensures that any branches to the
// merged return block are lexically forward.

insertionPoints[index] = returnBlock;
}
}
}
Expand All @@ -8493,6 +8548,9 @@ class MergedReturns
assert(searchLimit < maxReturns);
mergedReturnBlock = CreateReturnBB(searchLimit);
comp->genReturnBB = mergedReturnBlock;
// Downstream code expects the `genReturnBB` to always remain
// once created, so that it can redirect flow edges to it.
mergedReturnBlock->bbFlags |= BBF_DONT_REMOVE;
}
}

Expand Down Expand Up @@ -8563,11 +8621,13 @@ class MergedReturns
// searching for.
// searchLimit - Check `returnBlocks`/`returnConstants` up to but not including
// this index.
// index - [out] Index of return block in the `returnBlocks` array, if found;
// searchLimit otherwise.
//
// Return Value:
// A block that returns the same constant, if one is found; otherwise nullptr.
//
BasicBlock* FindConstReturnBlock(GenTreeIntConCommon* constExpr, unsigned searchLimit)
BasicBlock* FindConstReturnBlock(GenTreeIntConCommon* constExpr, unsigned searchLimit, unsigned* index)
{
ssize_t constVal = constExpr->IconValue();

Expand All @@ -8588,10 +8648,12 @@ class MergedReturns
continue;
}

*index = i;
return returnBlock;
}
}

*index = searchLimit;
return nullptr;
}
};
Expand Down Expand Up @@ -8760,6 +8822,8 @@ void Compiler::fgAddInternal()
}
}

merger.PlaceReturns();

if (info.compCallUnmanaged != 0)
{
// The P/Invoke helpers only require a frame variable, so only allocate the
Expand Down Expand Up @@ -15280,7 +15344,15 @@ void Compiler::fgReorderBlocks()
/* (bPrev is known to be a normal block at this point) */
if (!isRare)
{
reorderBlock = false;
if ((bDest == block->bbNext) && (block->bbJumpKind == BBJ_RETURN) && (bPrev->bbJumpKind == BBJ_ALWAYS))
{
// This is a common case with expressions like "return Expr1 && Expr2" -- move the return
// to establish fall-through.
}
else
{
reorderBlock = false;
}
}
else
{
Expand Down
82 changes: 82 additions & 0 deletions tests/src/JIT/opt/Loops/SearchLoopTail.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,82 @@ static int Distance_abCD(string s)
return -1;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static bool TrueFalseOr(int[] input)
{
int n = 0, m = 0;
for (int i = 0; i < input.Length; ++i)
{
m = n;
n = input[i];
if (n < 7)
return true;
if (n > 22)
return false;
if (n == 12)
return (m == 5);
}

return (n == 9 || m == 13);
}

[MethodImpl(MethodImplOptions.NoInlining)]
static bool TrueFalseAnd(int[] input)
{
int n = 0, m = 0;
for (int i = 0; i < input.Length; ++i)
{
m = n;
n = input[i];
if (n < 7)
return true;
if (n > 22)
return false;
if (n == 12)
return (m == 5);
}

return (n == 9 && m == 13);
}

[MethodImpl(MethodImplOptions.NoInlining)]
static bool FalseTrueOr(int[] input)
{
int n = 0, m = 0;
for (int i = 0; i < input.Length; ++i)
{
m = n;
n = input[i];
if (n < 7)
return false;
if (n > 22)
return true;
if (n == 12)
return (m == 5);
}

return (n == 9 || m == 13);
}

[MethodImpl(MethodImplOptions.NoInlining)]
static bool FalseTrueAnd(int[] input)
{
int n = 0, m = 0;
for (int i = 0; i < input.Length; ++i)
{
m = n;
n = input[i];
if (n < 7)
return false;
if (n > 22)
return true;
if (n == 12)
return (m == 5);
}

return (n == 9 && m == 13);
}

public static int Main(string[] args)
{
if (HasPrimeUnderTwenty(22, 36) || !HasPrimeUnderTwenty(-1, 4))
Expand All @@ -72,6 +148,12 @@ public static int Main(string[] args)
return -1;
}

int[] inputs = new int[] { 8, 20, 11, 13, 15 };
if (!TrueFalseOr(inputs) || TrueFalseAnd(inputs) || !FalseTrueOr(inputs) || FalseTrueAnd(inputs))
{
return -1;
}

return 100;
}
}
Expand Down

0 comments on commit 20ac1a9

Please sign in to comment.