diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 4851dd5a90e31f..95f7537f145695 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -1015,6 +1015,38 @@ bool BasicBlock::isEmpty() const return true; } +//------------------------------------------------------------------------ +// hasSideEffects: check if block has side effects +// +// Returns: +// True if any non-phi statement or node in the block has side effects. +// +bool BasicBlock::hasSideEffects() const +{ + if (!IsLIR()) + { + for (Statement* const stmt : NonPhiStatements()) + { + if ((stmt->GetRootNode()->gtFlags & GTF_SIDE_EFFECT) != 0) + { + return true; + } + } + } + else + { + for (GenTree* node : LIR::AsRange(this)) + { + if ((node->gtFlags & GTF_SIDE_EFFECT) != 0) + { + return true; + } + } + } + + return false; +} + //------------------------------------------------------------------------ // isValid: Checks that the basic block doesn't mix statements and LIR lists. // diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 40d6e0f1f62bea..15595d5462bc56 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -1768,6 +1768,10 @@ struct BasicBlock : private LIR::Range return StatementList(FirstNonPhiDef()); } + // True if any non-phi statement/node in the block has side effects. + // + bool hasSideEffects() const; + // Simple "size" estimates // unsigned StatementCount(); diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 79d46b87601ec6..15c40171929d29 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7836,6 +7836,7 @@ class Compiler // PhaseStatus optRedundantBranches(); bool optRedundantRelop(BasicBlock* const block); + bool optRedundantDominatingBranch(BasicBlock* const block); bool optRedundantBranch(BasicBlock* const block); bool optJumpThreadDom(BasicBlock* const block, BasicBlock* const domBlock, bool domIsSameRelop); bool optJumpThreadPhi(BasicBlock* const block, GenTree* tree, ValueNum treeNormVN); diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 2c1d5a35a9783c..a9f305bfcaa490 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -54,6 +54,11 @@ PhaseStatus Compiler::optRedundantBranches() madeChangesThisBlock |= m_compiler->optRedundantBranch(block); + if (block->KindIs(BBJ_COND)) + { + madeChangesThisBlock |= m_compiler->optRedundantDominatingBranch(block); + } + // If we modified some flow out of block but it's still referenced and // a BBJ_COND, retry; perhaps one of the later optimizations // we can do has enabled one of the earlier optimizations. @@ -709,6 +714,324 @@ bool Compiler::optRelopTryInferWithOneEqualOperand(const VNFuncApp& domApp, return true; } +//------------------------------------------------------------------------ +// optRedundantDominatingBranch: see if we can optimize a branch in a +// dominating block. +// +// Arguments: +// block - conditional block whose dominators will be probed for redundancy. +// +// Notes: +// This handles optimizing cases like +// +// if (x > 0) // block A, predicate pA +// if (x > 1) // block B, predicate pB +// S; +// +// into +// +// if (x > 1) +// S; +// +// by proving that pB ==> pA and that B is side effect free. +// +// We trigger this starting from block B with successors S and X, +// looking up at the immediate dominator A. If A branches to B and +// the other successor of A is either S or X, then we have the right +// control flow pattern for this optimization. +// +// Suppose X is the shared successor of A and B. +// +// We then see if the predicate for B->S implies the predicate for A->B. +// If so, and B is side effect free, we can change A to unconditionally +// branch to B. +// +// If this succeeds and A is side effect free, then we can look at the +// immediate dominator of A and repeat the process, potentially optimizing +// multiple dominating branches. +// +// Note that these dominating compares do not all have to share the +// same successor of B, that is if B's successors are S and X, then +// some A's can target S and others can target X. +// +// We may also want to make this be heuristic driven. If pA is +// likely false and B is expensive, this may not improve performance. +// +bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) +{ + if (!block->KindIs(BBJ_COND)) + { + return false; + } + + if (block->hasSideEffects()) + { + return false; + } + + Statement* const stmt = block->lastStmt(); + + if (stmt == nullptr) + { + return false; + } + + GenTree* const jumpTree = stmt->GetRootNode(); + + if (!jumpTree->OperIs(GT_JTRUE)) + { + return false; + } + + GenTree* const tree = jumpTree->AsOp()->gtOp1; + + if (!tree->OperIsCompare()) + { + return false; + } + + const ValueNum treeNormVN = vnStore->VNNormalValue(tree->GetVN(VNK_Liberal)); + + if (vnStore->IsVNConstant(treeNormVN)) + { + return false; + } + + // Skip through chains of empty or side effect free blocks. + // Watch for cycles. + // + auto skipSideEffectFreeBlocks = [=](BasicBlock* b) { + BitVecTraits traits(fgBBNumMax + 1, this); + BitVec visitedBlocks = BitVecOps::MakeEmpty(&traits); + while (!b->hasSideEffects() && b->KindIs(BBJ_ALWAYS)) + { + b = b->GetUniqueSucc(); + + if (!BitVecOps::TryAddElemD(&traits, visitedBlocks, b->bbNum)) + { + // Block is already visited, we have a cycle. Bail out. + break; + } + } + + return b; + }; + + BasicBlock* const blockTrueSucc = skipSideEffectFreeBlocks(block->GetTrueTarget()); + BasicBlock* const blockFalseSucc = skipSideEffectFreeBlocks(block->GetFalseTarget()); + BasicBlock* currentBlock = block; + BasicBlock* domBlockProbe = fgGetDomSpeculatively(block); + ValueNum blockPathVN = ValueNumStore::NoVN; + bool madeChanges = false; + unsigned searchCount = 0; + const unsigned searchLimit = 8; + + JITDUMP("Checking " FMT_BB " for redundant dominating branches\n", block->bbNum); + + if (domBlockProbe == nullptr) + { + JITDUMP("failed -- no dominator\n") + } + + // Walk up the dominator tree. + // We may be able to optimize multiple dominating branches. + // + while (domBlockProbe != nullptr) + { + // Avoid walking too far up long skinny dominator trees. + // + searchCount++; + + if (searchCount > searchLimit) + { + JITDUMP("stopping, hit search limit\n"); + break; + } + + // Skip past unconditional dominators, if any, as long as they + // do not have side effects (since they may now become unconditionally + // executed along the path to block). + // + while ((domBlockProbe != nullptr) && domBlockProbe->KindIs(BBJ_ALWAYS)) + { + if (domBlockProbe->GetTarget() != currentBlock) + { + domBlockProbe = nullptr; + break; + } + + if (domBlockProbe->hasSideEffects()) + { + domBlockProbe = nullptr; + break; + } + + currentBlock = domBlockProbe; + domBlockProbe = fgGetDomSpeculatively(domBlockProbe); + } + + if (domBlockProbe == nullptr) + { + JITDUMP("failed -- no dominator\n"); + break; + } + + if (!domBlockProbe->KindIs(BBJ_COND)) + { + JITDUMP("failed -- dominator " FMT_BB " is not BBJ_COND\n", domBlockProbe->bbNum); + break; + } + + // Make sure this conditional dominator branches to the same + // shared block as the original block. + // + BasicBlock* const domTrueSucc = skipSideEffectFreeBlocks(domBlockProbe->GetTrueTarget()); + BasicBlock* const domFalseSucc = skipSideEffectFreeBlocks(domBlockProbe->GetFalseTarget()); + + const bool currentIsDomTrueSucc = (domTrueSucc == currentBlock); + const bool currentIsDomFalseSucc = (domFalseSucc == currentBlock); + + if (currentIsDomTrueSucc == currentIsDomFalseSucc) + { + JITDUMP("failed -- " FMT_BB " is degnerate\n", domBlockProbe->bbNum); + // degenerate BBJ_COND + break; + } + + BasicBlock* const sharedSuccessor = currentIsDomTrueSucc ? domFalseSucc : domTrueSucc; + + // Find the VN for the path from block to the non-shared successor. + // + if (sharedSuccessor == blockFalseSucc) + { + // Shared successor is block's false successor, so unshared successor is block's true successor. + // Thus the path from block to the unshared successor corresponds to the relop being true. + // + blockPathVN = treeNormVN; + } + else if (sharedSuccessor == blockTrueSucc) + { + // Shared successor is block's true successor, so unshared successor is block's false successor. + // Thus the path from block to the unshared successor corresponds to the relop being false. + // + blockPathVN = vnStore->GetRelatedRelop(treeNormVN, ValueNumStore::VN_RELATION_KIND::VRK_Reverse); + } + else + { + JITDUMP("failed -- " FMT_BB " does not share a successor with " FMT_BB "\n", domBlockProbe->bbNum, + block->bbNum); + break; + } + + if (blockPathVN == ValueNumStore::NoVN) + { + JITDUMP("failed -- " FMT_BB " does not have a usable VN\n", block->bbNum); + break; + } + + JITDUMP(FMT_BB " and " FMT_BB " have shared successor " FMT_BB "\n", domBlockProbe->bbNum, block->bbNum, + sharedSuccessor->bbNum); + + // Find the VN for the path from domBlockProbe to block. + // + Statement* const domStmt = domBlockProbe->lastStmt(); + assert(domStmt != nullptr); + + GenTree* const domJumpTree = domStmt->GetRootNode(); + assert(domJumpTree->OperIs(GT_JTRUE)); + + GenTree* const domTree = domJumpTree->AsOp()->gtGetOp1(); + + if (!domTree->OperIsCompare()) + { + break; + } + + const ValueNum domNormVN = vnStore->VNNormalValue(domTree->GetVN(VNK_Liberal)); + + if (vnStore->IsVNConstant(domNormVN)) + { + break; + } + + ValueNum domPathVN = domNormVN; + + if (currentIsDomFalseSucc) + { + domPathVN = vnStore->GetRelatedRelop(domPathVN, ValueNumStore::VN_RELATION_KIND::VRK_Reverse); + } + + if (domPathVN == ValueNumStore::NoVN) + { + break; + } + + // We found a dominating compare with the right pattern of control flow. + // See if the block's path relop implies the dom's path relop. + // + RelopImplicationInfo rii; + rii.treeNormVN = domPathVN; + rii.domCmpNormVN = blockPathVN; + + optRelopImpliesRelop(&rii); + + if (!(rii.canInfer && rii.canInferFromTrue && !rii.reverseSense)) + { + JITDUMP("failed -- Dominated VN " FMT_VN " does not imply dominating VN " FMT_VN "\n", blockPathVN, + domPathVN); + break; + } + + JITDUMP("Optimizing branch in dominating " FMT_BB " with relop [%06u] based on " FMT_BB "'s relop [%06u]\n", + domBlockProbe->bbNum, dspTreeID(domTree), block->bbNum, dspTreeID(tree)); + + const int domRelopValue = currentIsDomTrueSucc ? 1 : 0; + + bool domMayHaveSideEffects = false; + + // Always preserve side effects in the dominating relop. + // + if ((domTree->gtFlags & GTF_SIDE_EFFECT) != 0) + { + JITDUMP("Dominating relop has side effects, keeping it, unused\n"); + GenTree* const relopComma = gtNewOperNode(GT_COMMA, TYP_INT, domTree, gtNewIconNode(domRelopValue)); + domJumpTree->AsUnOp()->gtOp1 = relopComma; + domMayHaveSideEffects = true; + } + else + { + domTree->BashToConst(domRelopValue); + } + + JITDUMP("\nRedundant dominating branch opt in " FMT_BB ":\n", domBlockProbe->bbNum); + + fgMorphBlockStmt(domBlockProbe, domStmt DEBUGARG(__FUNCTION__), /* allowFGChange */ true, + /* invalidateDFSTreeOnFGChange */ false); + Metrics.RedundantBranchesEliminated++; + madeChanges = true; + + // We can keep looking if we haven't seen any side effects yet along the path to block. + // + if (!domMayHaveSideEffects) + { + domMayHaveSideEffects = domBlockProbe->hasSideEffects(); + } + + if (domMayHaveSideEffects) + { + JITDUMP("stopping -- side effects seen along path to block\n"); + break; + } + + currentBlock = domBlockProbe; + domBlockProbe = fgGetDomSpeculatively(domBlockProbe); + + JITDUMP("continuing to the next immediate dominator\n"); + } + + return madeChanges; +} + //------------------------------------------------------------------------ // optRedundantBranch: try and optimize a possibly redundant branch // diff --git a/src/tests/JIT/opt/RedundantBranch/RedundantBranchDominating.cs b/src/tests/JIT/opt/RedundantBranch/RedundantBranchDominating.cs new file mode 100644 index 00000000000000..ecd777b6bb6a7f --- /dev/null +++ b/src/tests/JIT/opt/RedundantBranch/RedundantBranchDominating.cs @@ -0,0 +1,144 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; +using Xunit; + +// Runtime 126554 +// These cases specifically exercise redundant dominating branch elimination: +// the innermost test implies one or more dominating tests and all of them share +// the same untaken exit. +public class RedundantBranchDominating +{ + private static int s_effects; + private static readonly int[] s_values = { -10, -3, -2, -1, 0, 1, 2, 3, 10, 499, 500 }; + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static int SideEffect(int value) + { + s_effects++; + return value; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int Dom_00(int count) + { + if ((SideEffect(count) > 0) && (count < 500)) + { + if (count == 10) + { + return 1; + } + + return 3; + } + + return 3; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int Dom_01(int count) + { + if (SideEffect(count) > 0) + { + if (count > 1) + { + if (count > 2) + { + return 1; + } + } + } + + return 3; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int Dom_02(int count) + { + if (SideEffect(count) > 0) + { + return 3; + } + + if (count < -1) + { + if (count < -2) + { + return 1; + } + } + + return 3; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int Dom_03(int count) + { + if (count > 0) + { + if (SideEffect(count) > 1) + { + if (count > 2) + { + return 1; + } + } + } + + return 3; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int Dom_04(int count) + { + if ((count > 1) || (count > 0)) + { + if (count > 0) + { + return 1; + } + + return 2; + } + + return 3; + } + + private static void RunTest(string name, Func func, int[] expectedResults, int[] expectedEffects) + { + s_effects = 0; + + for (int i = 0; i < s_values.Length; i++) + { + int value = s_values[i]; + int before = s_effects; + int result = func(value); + int effects = s_effects - before; + + Assert.True(result == expectedResults[i], $"{name}({value}) = {result}, expected {expectedResults[i]}"); + Assert.True(effects == expectedEffects[i], $"{name}({value}) effects={effects}, expected {expectedEffects[i]}"); + } + } + + [Fact] + public static void TestDom00() => + RunTest(nameof(Dom_00), Dom_00, new[] { 3, 3, 3, 3, 3, 3, 3, 3, 1, 3, 3 }, new[] { 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 }); + + [Fact] + public static void TestDom01() => + RunTest(nameof(Dom_01), Dom_01, new[] { 3, 3, 3, 3, 3, 3, 3, 1, 1, 1, 1 }, new[] { 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 }); + + [Fact] + public static void TestDom02() => + RunTest(nameof(Dom_02), Dom_02, new[] { 1, 1, 3, 3, 3, 3, 3, 3, 3, 3, 3 }, new[] { 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 }); + + [Fact] + public static void TestDom03() => + RunTest(nameof(Dom_03), Dom_03, new[] { 3, 3, 3, 3, 3, 3, 3, 1, 1, 1, 1 }, new[] { 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1 }); + + [Fact] + public static void TestDom04() => + RunTest(nameof(Dom_04), Dom_04, new[] { 3, 3, 3, 3, 3, 1, 1, 1, 1, 1, 1 }, new[] { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }); +} diff --git a/src/tests/JIT/opt/RedundantBranch/RedundantBranchDominating.csproj b/src/tests/JIT/opt/RedundantBranch/RedundantBranchDominating.csproj new file mode 100644 index 00000000000000..de6d5e08882e86 --- /dev/null +++ b/src/tests/JIT/opt/RedundantBranch/RedundantBranchDominating.csproj @@ -0,0 +1,8 @@ + + + True + + + + +