Use sameEHRegion guard

dotnet · Oct 16, 2019 · a8fcbb5 · a8fcbb5
1 parent 5c1dd65
commit a8fcbb5
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 7 deletions.
diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp
@@ -8946,7 +8946,7 @@ void Compiler::fgExpandBoolReturns()
     // Now look for BBJ_RETURN with non-constant expressions, 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)
+        if ((block->bbJumpKind != BBJ_RETURN) || (block->firstStmt() == nullptr) || (block->bbFlags & BBF_HAS_JMP) != 0)
         {
             continue;
         }
@@ -8956,7 +8956,7 @@ void Compiler::fgExpandBoolReturns()
             (returnOp->gtFlags & GTF_RET_MERGED) == 0)
         {
             GenTree* returnExpOp = returnOp->gtGetOp1();
-            if (!(returnExpOp->OperIsCompare()) || varTypeIsFloating(returnExpOp->gtGetOp1()->TypeGet()))
+            if (!(returnExpOp->OperIsCompare() || returnExpOp->OperIs(GT_NEG)) || varTypeIsFloating(returnExpOp->gtGetOp1()->TypeGet()))
             {
                 continue;
             }
@@ -8972,12 +8972,19 @@ void Compiler::fgExpandBoolReturns()
             {
                 // Inverse op so we can target the existing return false bb
                 // e.g. x == 0 -> x != 0
-                if (!(returnExpOp->OperIs(GT_EQ, GT_NE)))
+                if (!(BasicBlock::sameEHRegion(block, retFalseBb)) || !(returnExpOp->OperIs(GT_EQ, GT_NE, GT_GT, GT_GE, GT_LT, GT_LE, GT_TEST_EQ, GT_TEST_NE)))
                 {
                     continue;
                 }
                 returnExpOp->ChangeOper(GenTree::ReverseRelop(returnExpOp->OperGet()), GenTree::PRESERVE_VN);
-            } // if retTrueBb != nullptr we'll just redirect current one to it (no need to reverse the op)
+            }
+            else if (!BasicBlock::sameEHRegion(block, retTrueBb))
+            {
+                continue;
+            }
+
+            // TODO: check costs and only expand if cheap?
+            // TODO: check side effects flags?
 
             // Change BBJ_RETURN to BBJ_COND and jump to the existing retTrueBb or retFalseBb
             block->bbJumpKind = BBJ_COND;
@@ -8997,7 +9004,7 @@ void Compiler::fgExpandBoolReturns()
 
             // Create a basic block with just `return true(false);` after the current block
             // but first, check if there is already one there
-            bool expectedReturnResult = retTrueBb != nullptr ? false : true;
+            const bool expectedReturnResult = retTrueBb != nullptr ? false : true;
             bool nextBbReturnValue;
             if (!isReturnBoolConst(block->bbNext, &nextBbReturnValue) || nextBbReturnValue != expectedReturnResult)
             {

diff --git a/src/jit/optimizer.cpp b/src/jit/optimizer.cpp
@@ -8910,15 +8910,16 @@ void Compiler::optMergeBoolReturns()
                 continue;
             }
 
-            if (!returnOp->gtGetOp1()->OperIsCompare() || varTypeIsFloating(returnOp->gtGetOp1()->TypeGet()))
+            if (!(returnOp->gtGetOp1()->OperIsCompare() || returnOp->gtGetOp1()->OperIs(GT_NEG)) ||
+                varTypeIsFloating(returnOp->gtGetOp1()->TypeGet()))
             {
                 continue;
             }
 
             if (!jumpBbReturnValue)
             {
                 // if jump target bb is `return false` we need to inverse comparison for our future `return expr`
-                if (!returnOp->gtGetOp1()->OperIs(GT_EQ, GT_NE, GT_GT, GT_GE, GT_LT, GT_LE, GT_TEST_EQ, GT_TEST_NE))
+                if (!returnOp->gtGetOp1()->OperIs(GT_EQ, GT_NE))
                 {
                     continue;
                 }