From 5deb018677a5e62ad7f2e8183604b3461fa144d7 Mon Sep 17 00:00:00 2001 From: Joseph Tremoulet Date: Mon, 11 Sep 2017 14:56:16 -0400 Subject: [PATCH] Improve const return block placement 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. --- src/jit/flowgraph.cpp | 80 ++++++++++++++++++++-- tests/src/JIT/opt/Loops/SearchLoopTail.cs | 82 +++++++++++++++++++++++ 2 files changed, 158 insertions(+), 4 deletions(-) diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp index 9d6959529f1c..4152ae4ebf50 100644 --- a/src/jit/flowgraph.cpp +++ b/src/jit/flowgraph.cpp @@ -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 maxReturns; @@ -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 @@ -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); @@ -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) { @@ -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; } } } @@ -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; } } @@ -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(); @@ -8588,10 +8648,12 @@ class MergedReturns continue; } + *index = i; return returnBlock; } } + *index = searchLimit; return nullptr; } }; @@ -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 @@ -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 { diff --git a/tests/src/JIT/opt/Loops/SearchLoopTail.cs b/tests/src/JIT/opt/Loops/SearchLoopTail.cs index 1393bbec1931..10be60c02155 100644 --- a/tests/src/JIT/opt/Loops/SearchLoopTail.cs +++ b/tests/src/JIT/opt/Loops/SearchLoopTail.cs @@ -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)) @@ -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; } }