JIT: some enhancements to redundant branch opts (dotnet#60732)

Handle cases where the dominating compare is the reverse of the compare
we're trying to optimize. For example, if `(x > y)` dominates `(y <= x)`
we may be able to optimize the dominated compare.

Addresses aspects of dotnet#48115.

Author: AndyAyersMS

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

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);

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"?

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.