Expand BBJ_RETURN blocks with bool conditions

src/jit/compiler.cpp

@@ -4662,6 +4662,11 @@ void Compiler::compCompile(void** methodCodePtr, ULONG* methodCodeSize, JitFlags
         /* Optimize boolean conditions */
 
         optOptimizeBools();
+
+        if (info.compRetType == TYP_BOOL)
+        {
+            optMergeBoolReturns();
+        }
         EndPhase(PHASE_OPTIMIZE_BOOLS);
 
         // optOptimizeBools() might have changed the number of blocks; the dominators/reachability might be bad.

src/jit/compiler.h

@@ -4391,6 +4391,7 @@ class Compiler
 
     void fgMorphStmts(BasicBlock* block, bool* lnot, bool* loadw);
     void fgMorphBlocks();
+    void fgExpandBoolReturns();
 
     bool fgMorphBlockStmt(BasicBlock* block, Statement* stmt DEBUGARG(const char* msg));
 
@@ -5739,6 +5740,10 @@ class Compiler
 public:
     void optOptimizeBools();
 
+    bool isReturnBoolConst(BasicBlock* block, bool& value);
+
+    void optMergeBoolReturns();
+
 private:
     GenTree* optIsBoolCond(GenTree* condBranch, GenTree** compPtr, bool* boolPtr);
 #ifdef DEBUG

src/jit/flowgraph.cpp

@@ -8890,6 +8890,86 @@ class MergedReturns
 };
 }
 
+//-------------------------------------------------------------------------
+//  fgExpandBoolReturns: find all Bool BBJ_RETURN basic blocks and expand
+//  them into BBJ_COND if it's possible to re-use existing `return true/false`
+//  blocks. E.g.:
+//
+//  ReturnFalse:       // used by someone else
+//      return false;
+//  return x == 0;
+//
+//  to:
+//
+//  ReturnFalse:
+//      return false;
+//  if (x != 0)
+//      goto ReturnFalse;
+//  return true;
+//
+void Compiler::fgExpandBoolReturns()
+{
+    // Now look for BBJ_RETURN with a comparison, e.g. `return x == 0`
+    for (BasicBlock* block = fgFirstBB; block; block = block->bbNext)
+    {
+        if ((block->bbJumpKind != BBJ_RETURN) || (block->firstStmt() == nullptr) || (block->bbFlags & BBF_HAS_JMP) != 0)
+        {
+            continue;
+        }
+
+        GenTree* returnOp = block->firstStmt()->GetRootNode();
+        if (block->firstStmt()->GetNextStmt() == nullptr && returnOp->OperIs(GT_RETURN) &&
+            (returnOp->gtFlags & GTF_RET_MERGED) == 0)
+        {
+            GenTree* returnExpOp = returnOp->gtGetOp1();
+            if (!(returnExpOp->OperIsCompare()) || varTypeIsFloating(returnExpOp->gtGetOp1()->TypeGet()))
+            {
+                continue;
+            }
+
+            bool bbPrevJumpCnsValue = false;
+
+            // Expand only if previous block is also a BBJ_COND and jumps to a bool const.
+            if (block->bbPrev == nullptr || block->bbPrev->bbJumpKind != BBJ_COND ||
+                !isReturnBoolConst(block->bbPrev->bbJumpDest, bbPrevJumpCnsValue) ||
+                !BasicBlock::sameEHRegion(block, block->bbPrev))
+            {
+                continue;
+            }
+
+            bool bbNextCnsValue = false;
+            const bool bbNextIsCns = isReturnBoolConst(block->bbNext, bbNextCnsValue);
+
+            if (!bbPrevJumpCnsValue)
+            {
+                // Inverse op so we can target the existing 'return false'
+                // only if `return false` is not block->bbNext (in this case we don't have to reverse)
+                returnExpOp->ChangeOper(GenTree::ReverseRelop(returnExpOp->OperGet()), GenTree::PRESERVE_VN);
+            }
+
+            // Change BBJ_RETURN to BBJ_COND and jump to the existing retTrueBb or retFalseBb
+            block->bbJumpKind = BBJ_COND;
+            block->bbJumpDest = block->bbPrev->bbJumpDest;
+            returnOp->ChangeOperUnchecked(GT_JTRUE);
+            returnOp->ChangeType(TYP_VOID);
+            returnExpOp->gtFlags |= GTF_RELOP_JMP_USED;
+
+            // Create a basic block with just `return true(false);` after the current block
+            // but first, check if there is already one there
+            if (!bbNextIsCns || bbNextCnsValue == bbPrevJumpCnsValue)
+            {
+                BasicBlock* retBoolBlk = fgNewBBafter(BBJ_RETURN, block, true);
+                retBoolBlk->bbFlags |= BBF_JMP_TARGET | BBF_INTERNAL;
+                retBoolBlk->setBBWeight(block->bbWeight);
+                GenTreeIntCon* boolCns = gtNewIconNode(bbPrevJumpCnsValue ? 0 : 1);
+                fgInsertStmtAtEnd(retBoolBlk,
+                                  gtNewStmt(gtNewOperNode(GT_RETURN, genActualType(info.compRetType), boolCns)));
+            }
+            fgAddRefPred(block->bbPrev->bbJumpDest, block);
+        }
+    }
+}
+
 /*****************************************************************************
 *
 *  Add any internal blocks/trees we may need
@@ -8907,6 +8987,11 @@ void Compiler::fgAddInternal()
         fgFirstBB->bbFlags |= BBF_DONT_REMOVE;
     }
 
+    if (info.compRetType == TYP_BOOL)
+    {
+        fgExpandBoolReturns();
+    }
+
     /*
     <BUGNUM> VSW441487 </BUGNUM>
 

src/jit/optimizer.cpp

@@ -8794,6 +8794,140 @@ GenTree* Compiler::optIsBoolCond(GenTree* condBranch, GenTree** compPtr, bool* b
     return opr1;
 }
 
+//-------------------------------------------------------------------------
+//  isBlockReturnBoolConst: is basic block just for `return true/false`
+//
+//  Arguments:
+//      block      The Basic Block to check
+//      value      The Boolean value it returns, e.g. true for `return true`
+//
+//  Return Value:
+//      Returns true if the basic block only consists of a single statement with a single
+//      GT_RETURN CNS_INT 0/1 operation
+//
+bool Compiler::isReturnBoolConst(BasicBlock* block, bool& value)
+{
+    if ((block == nullptr) || (block->bbJumpKind != BBJ_RETURN) || (block->firstStmt() == nullptr))
+    {
+        return false;
+    }
+    GenTree* returnOp = block->firstStmt()->GetRootNode();
+    if (block->firstStmt()->GetNextStmt() == nullptr && returnOp->OperIs(GT_RETURN))
+    {
+        if (returnOp->gtGetOp1()->IsIntegralConst(0))
+        {
+            value = false;
+            return true;
+        }
+        else if (returnOp->gtGetOp1()->IsIntegralConst(1))
+        {
+            value = true;
+            return true;
+        }
+        // Can be just (?)
+        // value = (INT8)(returnOp->gtGetOp1()->AsIntCon()->IconValue) > 0;
+    }
+    return false;
+}
+
+//-------------------------------------------------------------------------
+//  optMergeBoolReturns: find all BBJ_COND basic block with `return true/false` tails.
+//  e.g.
+//
+//  if (expr)             // BBJ_COND with GT_JTRUE node
+//      goto ReturnTrue;  // block->bbJumpDest
+//  goto ReturnTrue;      // block->bbNext
+//
+//  ReturnTrue:           // BBJ_RETURN with GT_RETURN CNS_INT 1
+//      return true;
+//  ReturnFalse:          // BBJ_RETURN with GT_RETURN CNS_INT 0
+//      return true;
+//
+// And transform it into just `return expr` (or `return !expr` if block->bbJumpDest is return false)
+//
+void Compiler::optMergeBoolReturns()
+{
+#ifdef DEBUG
+    if (verbose)
+    {
+        printf("*************** In optMergeBoolReturns()\n");
+        if (verboseTrees)
+        {
+            printf("Blocks/Trees before phase\n");
+            fgDispBasicBlocks(true);
+        }
+    }
+#endif
+    assert(info.compRetType == TYP_BOOL);
+    for (BasicBlock* block = fgFirstBB; block; block = block->bbNext)
+    {
+        if ((block->bbJumpKind != BBJ_COND) || (block->firstStmt() == nullptr))
+        {
+            continue;
+        }
+
+        GenTree* returnOp = block->firstStmt()->GetRootNode();
+        if (returnOp->OperIs(GT_JTRUE))
+        {
+            bool nextBbReturnValue;
+            bool jumpBbReturnValue;
+
+            // are bbNext and bbJumpDest `return true/false`?
+            if (!isReturnBoolConst(block->bbNext, nextBbReturnValue) ||
+                !isReturnBoolConst(block->bbJumpDest, jumpBbReturnValue))
+            {
+                continue;
+            }
+
+            assert(block->bbNext != block->bbJumpDest);
+
+            if (nextBbReturnValue == jumpBbReturnValue)
+            {
+                // both branches are the same, e.g.:
+                // if (expr)
+                //     return true;
+                // else
+                //     return true;
+                continue;
+            }
+
+            if ((block->bbNext->countOfInEdges() != 1) ||
+                (!nextBbReturnValue && block->bbJumpDest->countOfInEdges() != 1))
+            {
+                // both blocks are used by someone else so it will increase binary size
+                // e.g. `jmp` vs `sete+movzx+ret`. However it probably improves performance so it's a trade-off
+                continue;
+            }
+
+            if (!(returnOp->gtGetOp1()->OperIsCompare()) ||
+                varTypeIsFloating(returnOp->gtGetOp1()->TypeGet()))
+            {
+                continue;
+            }
+
+            if (!jumpBbReturnValue)
+            {
+                returnOp->gtGetOp1()->ChangeOper(GenTree::ReverseRelop(returnOp->gtGetOp1()->OperGet()),
+                                                 GenTree::PRESERVE_VN);
+            }
+
+            // remove self from the lists of predecessors for block->bbJumpDest and block->bbNext
+            // these blocks are now probably unreachable and will be deleted
+            fgRemoveRefPred(block->bbJumpDest, block);
+            fgRemoveRefPred(block->bbNext, block);
+
+            returnOp->gtGetOp1()->gtFlags &= ~(GTF_RELOP_JMP_USED);
+            returnOp->ChangeOperUnchecked(GT_RETURN); // was: GT_JTRUE
+            returnOp->ChangeType(TYP_INT);            // was: TYP_VOID
+            block->bbJumpKind = BBJ_RETURN;           // was: BBJ_COND
+            block->bbJumpDest = nullptr;
+        }
+    }
+#ifdef DEBUG
+    fgDebugCheckBBlist();
+#endif
+}
+
 void Compiler::optOptimizeBools()
 {
 #ifdef DEBUG