Address some CR feedback

Author: mikedn

src/jit/codegenxarch.cpp

@@ -1602,6 +1602,21 @@ static const GenConditionDesc& GetConditionDesc(GenCondition condition)
     return desc;
 }
 
+inline instruction ins_CMOV(emitJumpKind kind)
+{
+    assert((kind >= EJ_jo) && (kind <= EJ_jg));
+
+    // Spot check some EJ_/INS_ values to make sure they're ordered correctly.
+    static_assert_no_msg((EJ_jo - EJ_jo) + INS_cmovo == INS_cmovo);
+    static_assert_no_msg((EJ_jae - EJ_jo) + INS_cmovo == INS_cmovae);
+    static_assert_no_msg((EJ_jl - EJ_jo) + INS_cmovo == INS_cmovl);
+    static_assert_no_msg((EJ_jg - EJ_jo) + INS_cmovo == INS_cmovg);
+
+    instruction ins = static_cast<instruction>((kind - EJ_jo) + INS_cmovo);
+    assert(insIsCMOV(ins));
+    return ins;
+}
+
 //------------------------------------------------------------------------
 // genCodeForJcc: Produce code for a GT_JCC node.
 //
@@ -1712,23 +1727,16 @@ void CodeGen::genCodeForSelCC(GenTreeOpCC* selcc)
     if (compiler->opts.compUseCMOV)
     {
         const GenConditionDesc& desc = GetConditionDesc(selcc->gtCondition);
-        assert(desc.ins[1] == EJ_NONE);
-
-        constexpr instruction EJtoCMOV[]{INS_nop,    INS_nop,    INS_cmovo,  INS_cmovno, INS_cmovb,  INS_cmovae,
-                                         INS_cmove,  INS_cmovne, INS_cmovbe, INS_cmova,  INS_cmovs,  INS_cmovns,
-                                         INS_cmovpe, INS_cmovpo, INS_cmovl,  INS_cmovge, INS_cmovle, INS_cmovg};
-        noway_assert((unsigned)desc.ins[0] < _countof(EJtoCMOV));
-        instruction ins = EJtoCMOV[desc.ins[0]];
-        noway_assert(insIsCMOV(ins));
+        assert(desc.ins[1] == EJ_NONE); // Floating point conditions are not yet supported
 
-        getEmitter()->emitInsBinary(ins, emitTypeSize(dstType), op1, op2);
+        getEmitter()->emitInsBinary(ins_CMOV(desc.ins[0]), emitTypeSize(dstType), op1, op2);
     }
     else
     {
         GenCondition condition = selcc->gtCondition;
         condition.Reverse();
         const GenConditionDesc& desc = GetConditionDesc(condition);
-        assert(desc.ins[1] == EJ_NONE);
+        assert(desc.ins[1] == EJ_NONE); // Floating point conditions are not yet supported
 
         BasicBlock* label = genCreateTempLabel();
         inst_JMP(desc.ins[0], label);

src/jit/lower.cpp

@@ -3126,36 +3126,49 @@ class IfConversion
         m_falseBlock = m_block->bbNext;
         m_trueBlock  = m_block->bbJumpDest;
 
-        BasicBlock* joinBlock = SingleSuccessorBlock(m_falseBlock);
+        assert(m_falseBlock != m_block); // m_block can't fall through to itself
 
-        if (joinBlock == m_block->bbJumpDest)
+        if (m_trueBlock == m_falseBlock)
         {
-            if (LastInstruction(m_falseBlock) == nullptr)
+            // Reject degenerate full hammocks, fg optimizations should have taken care of this.
+            // They may end up passing as genuine full hammocks and cause various problems
+            // (e.g. decrement the ref count of a lclvar twice).
+            return HammockKind::None;
+        }
+
+        if (((m_trueBlock->bbFlags & BBF_RUN_RARELY) == 0) && ((m_block->bbFlags & BBF_RUN_RARELY) != 0))
+        {
+            // fall through from not-run-rarerly to run-rarely
+            return HammockKind::None;
+        }
+
+        if (m_trueBlock == m_block)
+        {
+            // Reject some back edges. A half hammock may be formed if "falseBlock" jumps back to
+            // "block" but that would be an infinite loop and it's not exactly common (and likely
+            // there's no test coverage for).
+            return HammockKind::None;
+        }
+
+        m_joinBlock = SingleSuccessorBlock(m_falseBlock);
+
+        if (m_joinBlock == m_trueBlock)
+        {
+            if (m_comp->fgInDifferentRegions(m_block, m_falseBlock))
             {
-                // The block is empty, fg optimizations should have taken care of this.
                 return HammockKind::None;
             }
 
-            if (m_comp->fgInDifferentRegions(m_block, m_falseBlock) || !BasicBlock::sameEHRegion(m_block, m_falseBlock))
+            if (!BasicBlock::sameEHRegion(m_block, m_falseBlock))
             {
                 return HammockKind::None;
             }
 
             // We have something like "if (cond) { falseBlock: ... } joinBlock: ..."
-
-            m_joinBlock = joinBlock;
             return HammockKind::Half;
         }
         else
         {
-            if ((LastInstruction(m_falseBlock) == nullptr) || (LastInstruction(m_trueBlock) == nullptr))
-            {
-                // Either or both blocks are empty, fg optimizations should have taken care of this.
-                // If only one block is empty we actually have a half hammock but to keep things
-                // simple this special case is not handled.
-                return HammockKind::None;
-            }
-
             if ((m_falseBlock->bbJumpKind == BBJ_RETURN) && (m_trueBlock->bbJumpKind == BBJ_RETURN))
             {
 #ifdef JIT32_GCENCODER
@@ -3165,14 +3178,26 @@ class IfConversion
                 // limit imposed by the x86 GC encoding.
                 return HammockKind::None;
 #endif
-                m_joinBlock = nullptr;
+                // SingleSuccessorBlock should have returned null for a BBJ_RETURN block
+                assert(m_joinBlock == nullptr);
             }
-            else if ((joinBlock != nullptr) && (joinBlock == SingleSuccessorBlock(m_trueBlock)))
+            else if ((m_joinBlock == nullptr) || (m_joinBlock != SingleSuccessorBlock(m_trueBlock)))
             {
-                m_joinBlock = joinBlock;
+                return HammockKind::None;
             }
-            else
+            else if (m_joinBlock == m_block)
+            {
+                // Reject full hammocks where both "false" and "true" blocks jump back to "block".
+                // That would be an infinite infinite loop and it's not exactly common (and likely
+                // there's no test coverage for).
+                return HammockKind::None;
+            }
+
+            if (!m_block->isRunRarely() && m_trueBlock->isRunRarely())
             {
+                // If we're jumping from a normal block to a rarely run block then it means we have
+                // a rarely taken branch and it's likely not beneficial to convert it.
+                // TODO What about fall through?
                 return HammockKind::None;
             }
 
@@ -3188,7 +3213,6 @@ class IfConversion
             }
 
             // We have something like "if (cond) { falseBlock: ... } else { trueBlock: ... } joinBlock: ..."
-
             return HammockKind::Full;
         }
     }
@@ -3221,12 +3245,18 @@ class IfConversion
 
             GenTree* op = LastInstruction(m_block);
 
+            if (op == nullptr)
+            {
+                // The block is empty, fg optimizations should have taken care of this.
+                return false;
+            }
+
             if ((kind == HammockKind::Half) && op->OperIs(GT_RETURN))
             {
                 return false;
             }
 
-            if ((op == nullptr) || !op->OperIs(GT_STORE_LCL_VAR, GT_RETURN))
+            if (!op->OperIs(GT_STORE_LCL_VAR, GT_RETURN))
             {
                 // TODO Indirs would also work provided that both blocks store to the same address.
                 // How to detect that?