diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 5d40dd03d29786..442e249abf91fb 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7453,7 +7453,7 @@ class Compiler // PhaseStatus optRedundantBranches(); bool optRedundantBranch(BasicBlock* const block); - bool optJumpThread(BasicBlock* const block, BasicBlock* const domBlock); + bool optJumpThread(BasicBlock* const block, BasicBlock* const domBlock, bool domIsSameRelop); bool optReachable(BasicBlock* const fromBlock, BasicBlock* const toBlock, BasicBlock* const excludedBlock); #if ASSERTION_PROP diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index 4afefd6e2a10af..c0a5e419920770 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -128,23 +128,42 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) if (domCmpTree->OperKind() & GTK_RELOP) { - // We can use liberal VNs as bounds checks are not yet + // We can use liberal VNs here, as bounds checks are not yet // manifest explicitly as relops. // - ValueNum domCmpVN = domCmpTree->GetVN(VNK_Liberal); - - // Note we could also infer the tree relop's value from similar relops higher in the dom tree. - // For example, (x >= 0) dominating (x > 0), or (x < 0) dominating (x > 0). + // Look for an exact match and also try the various swapped/reversed forms. + // + const ValueNum treeVN = tree->GetVN(VNK_Liberal); + const ValueNum domCmpVN = domCmpTree->GetVN(VNK_Liberal); + const ValueNum domCmpSwpVN = + vnStore->GetRelatedRelop(domCmpVN, ValueNumStore::VN_RELATION_KIND::VRK_Swap); + const ValueNum domCmpRevVN = + vnStore->GetRelatedRelop(domCmpVN, ValueNumStore::VN_RELATION_KIND::VRK_Reverse); + const ValueNum domCmpSwpRevVN = + vnStore->GetRelatedRelop(domCmpVN, ValueNumStore::VN_RELATION_KIND::VRK_SwapReverse); + + const bool matchCmp = ((domCmpVN != ValueNumStore::NoVN) && (domCmpVN == treeVN)); + const bool matchSwp = ((domCmpSwpVN != ValueNumStore::NoVN) && (domCmpSwpVN == treeVN)); + const bool matchRev = ((domCmpRevVN != ValueNumStore::NoVN) && (domCmpRevVN == treeVN)); + const bool matchSwpRev = ((domCmpSwpRevVN != ValueNumStore::NoVN) && (domCmpSwpRevVN == treeVN)); + const bool domIsSameRelop = matchCmp || matchSwp; + const bool domIsRevRelop = matchRev || matchSwpRev; + + // Note we could also infer the tree relop's value from relops higher in the dom tree + // that involve the same operands but are not swaps or reverses. + // + // For example, (x >= 0) dominating (x > 0). // // That is left as a future enhancement. // - if (domCmpVN == tree->GetVN(VNK_Liberal)) + if (domIsSameRelop || domIsRevRelop) { // The compare in "tree" is redundant. // Is there a unique path from the dominating compare? // - JITDUMP("\nDominator " FMT_BB " of " FMT_BB " has relop with same liberal VN:\n", domBlock->bbNum, - block->bbNum); + JITDUMP("\nDominator " FMT_BB " of " FMT_BB " has %srelop with %s liberal VN\n", domBlock->bbNum, + block->bbNum, matchSwp || matchSwpRev ? "swapped " : "", + domIsSameRelop ? "the same" : "a reverse"); DISPTREE(domCmpTree); JITDUMP(" Redundant compare; current relop:\n"); DISPTREE(tree); @@ -162,7 +181,7 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) // However we may be able to update the flow from block's predecessors so they // bypass block and instead transfer control to jump's successors (aka jump threading). // - const bool wasThreaded = optJumpThread(block, domBlock); + const bool wasThreaded = optJumpThread(block, domBlock, domIsSameRelop); if (wasThreaded) { @@ -171,20 +190,20 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) } else if (trueReaches) { - // Taken jump in dominator reaches, fall through doesn't; relop must be true. + // Taken jump in dominator reaches, fall through doesn't; relop must be true/false. // - JITDUMP("Jump successor " FMT_BB " of " FMT_BB " reaches, relop must be true\n", - domBlock->bbJumpDest->bbNum, domBlock->bbNum); - relopValue = 1; + JITDUMP("Jump successor " FMT_BB " of " FMT_BB " reaches, relop must be %s\n", + domBlock->bbJumpDest->bbNum, domBlock->bbNum, domIsSameRelop ? "true" : "false"); + relopValue = domIsSameRelop ? 1 : 0; break; } else if (falseReaches) { - // Fall through from dominator reaches, taken jump doesn't; relop must be false. + // Fall through from dominator reaches, taken jump doesn't; relop must be false/true. // - JITDUMP("Fall through successor " FMT_BB " of " FMT_BB " reaches, relop must be false\n", - domBlock->bbNext->bbNum, domBlock->bbNum); - relopValue = 0; + JITDUMP("Fall through successor " FMT_BB " of " FMT_BB " reaches, relop must be %s\n", + domBlock->bbNext->bbNum, domBlock->bbNum, domIsSameRelop ? "false" : "true"); + relopValue = domIsSameRelop ? 0 : 1; break; } else @@ -259,6 +278,8 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) // Arguments: // block - block with branch to optimize // domBlock - a dominating block that has an equivalent branch +// domIsSameRelop - if true, dominating block does the same compare; +// if false, dominating block does a reverse compare // // Returns: // True if the branch was optimized. @@ -273,7 +294,7 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) // / \ | | // C D C D // -bool Compiler::optJumpThread(BasicBlock* const block, BasicBlock* const domBlock) +bool Compiler::optJumpThread(BasicBlock* const block, BasicBlock* const domBlock, bool domIsSameRelop) { assert(block->bbJumpKind == BBJ_COND); assert(domBlock->bbJumpKind == BBJ_COND); @@ -370,8 +391,13 @@ bool Compiler::optJumpThread(BasicBlock* const block, BasicBlock* const domBlock // are correlated exclusively with a true value for block's relop, and which // are correlated exclusively with a false value (aka true preds and false preds). // - // To do this we try and follow the flow from domBlock to block; any block pred - // reachable from domBlock's true edge is a true pred, and vice versa. + // To do this we try and follow the flow from domBlock to block. When domIsSameRelop + // is true, any block pred reachable from domBlock's true edge is a true pred of block, + // and any block pred reachable from domBlock's false edge is a false pred of block. + // + // If domIsSameRelop is false, then the roles of the of the paths from domBlock swap: + // any block pred reachable from domBlock's true edge is a false pred of block, + // and any block pred reachable from domBlock's false edge is a true pred of block. // // However, there are some exceptions: // @@ -400,8 +426,8 @@ bool Compiler::optJumpThread(BasicBlock* const block, BasicBlock* const domBlock int numTruePreds = 0; int numFalsePreds = 0; BasicBlock* fallThroughPred = nullptr; - BasicBlock* const trueSuccessor = domBlock->bbJumpDest; - BasicBlock* const falseSuccessor = domBlock->bbNext; + BasicBlock* const trueSuccessor = domIsSameRelop ? domBlock->bbJumpDest : domBlock->bbNext; + BasicBlock* const falseSuccessor = domIsSameRelop ? domBlock->bbNext : domBlock->bbJumpDest; BasicBlock* const trueTarget = block->bbJumpDest; BasicBlock* const falseTarget = block->bbNext; BlockSet truePreds = BlockSetOps::MakeEmpty(this); diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 73c78b8b1d8a2f..d3a17814a1f490 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4450,6 +4450,161 @@ bool ValueNumStore::IsVNHandle(ValueNum vn) return c->m_attribs == CEA_Handle; } +//------------------------------------------------------------------------ +// GetRelatedRelop: return value number for reversed/swapped comparison +// +// Arguments: +// vn - vn to base things on +// vrk - whether the new vn should swap, reverse, or both +// +// Returns: +// vn for reversed/swapped comparsion, or NoVN. +// +// Note: +// If "vn" corresponds to (x > y), the resulting VN corresponds to +// VRK_Swap (y < x) +// VRK_Reverse (x <= y) +// VRK_SwapReverse (y >= x) +// +// Will return NoVN for all float comparisons. +// +ValueNum ValueNumStore::GetRelatedRelop(ValueNum vn, VN_RELATION_KIND vrk) +{ + if (vn == NoVN) + { + return NoVN; + } + + // Pull out any exception set. + // + ValueNum valueVN; + ValueNum excepVN; + VNUnpackExc(vn, &valueVN, &excepVN); + + // Verify we have a binary func application + // + VNFuncApp funcAttr; + if (!GetVNFunc(valueVN, &funcAttr)) + { + return NoVN; + } + + if (funcAttr.m_arity != 2) + { + return NoVN; + } + + // Don't try and model float compares. + // + if (varTypeIsFloating(TypeOfVN(funcAttr.m_args[0]))) + { + return NoVN; + } + + const bool reverse = (vrk == VN_RELATION_KIND::VRK_Reverse) || (vrk == VN_RELATION_KIND::VRK_SwapReverse); + const bool swap = (vrk == VN_RELATION_KIND::VRK_Swap) || (vrk == VN_RELATION_KIND::VRK_SwapReverse); + + // Set up the new function + // + VNFunc newFunc = funcAttr.m_func; + + // Swap the predicate, if so asked. + // + if (swap) + { + if (newFunc >= VNF_Boundary) + { + switch (newFunc) + { + case VNF_LT_UN: + newFunc = VNF_GT_UN; + break; + case VNF_LE_UN: + newFunc = VNF_GE_UN; + break; + case VNF_GE_UN: + newFunc = VNF_LE_UN; + break; + case VNF_GT_UN: + newFunc = VNF_LT_UN; + break; + default: + return NoVN; + } + } + else + { + const genTreeOps op = (genTreeOps)newFunc; + + if (!GenTree::OperIsRelop(op)) + { + return NoVN; + } + + newFunc = (VNFunc)GenTree::SwapRelop(op); + } + } + + // Reverse the predicate, if so asked. + // + if (reverse) + { + if (newFunc >= VNF_Boundary) + { + switch (newFunc) + { + case VNF_LT_UN: + newFunc = VNF_GE_UN; + break; + case VNF_LE_UN: + newFunc = VNF_GT_UN; + break; + case VNF_GE_UN: + newFunc = VNF_LT_UN; + break; + case VNF_GT_UN: + newFunc = VNF_LE_UN; + break; + default: + return NoVN; + } + } + else + { + const genTreeOps op = (genTreeOps)newFunc; + + if (!GenTree::OperIsRelop(op)) + { + return NoVN; + } + + newFunc = (VNFunc)GenTree::ReverseRelop(op); + } + } + + // Create the resulting VN, swapping arguments if needed. + // + ValueNum newVN = VNForFunc(TYP_INT, newFunc, funcAttr.m_args[swap ? 1 : 0], funcAttr.m_args[swap ? 0 : 1]); + ValueNum result = VNWithExc(newVN, excepVN); + +#ifdef DEBUG + if (m_pComp->verbose) + { + printf("%s of ", swap ? (reverse ? "swap-reverse" : "swap") : "reverse"); + m_pComp->vnPrint(vn, 1); + printf(" => "); + m_pComp->vnPrint(newVN, 1); + if (result != newVN) + { + m_pComp->vnPrint(result, 1); + } + printf("\n"); + } +#endif + + return result; +} + bool ValueNumStore::IsVNConstantBound(ValueNum vn) { // Do we have "var < 100"? diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 5c521fe5a8cbf5..03974f821efbae 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -842,6 +842,19 @@ class ValueNumStore // Returns true iff the VN represents a handle constant. bool IsVNHandle(ValueNum vn); + // Given VN(x > y), return VN(y > x), VN(x <= y) or VN(y >= x) + // + // If vn is not a relop, return NoVN. + // + enum class VN_RELATION_KIND + { + VRK_Swap, // (y > x) + VRK_Reverse, // (x <= y) + VRK_SwapReverse // (y >= x) + }; + + ValueNum GetRelatedRelop(ValueNum vn, VN_RELATION_KIND vrk); + // Convert a vartype_t to the value number's storage type for that vartype_t. // For example, ValueNum of type TYP_LONG are stored in a map of INT64 variables. // Lang is the language (C++) type for the corresponding vartype_t.