Eliminate redundant test instruction

src/coreclr/jit/clrjit.natvis

@@ -155,6 +155,14 @@ The .NET Foundation licenses this file to you under the MIT license.
     <DisplayString>IG{igNum,d}</DisplayString>
   </Type>
 
+  <Type Name="emitter::instrDesc">
+    <DisplayString Condition="(_idInsFmt == IF_RRD) || (_idInsFmt == IF_RWR) || (_idInsFmt == IF_RRW)">{_idIns,en} {_idReg1,en}</DisplayString>
+    <DisplayString Condition="(_idInsFmt == IF_RRD_CNS) || (_idInsFmt == IF_RWR_CNS) || (_idInsFmt == IF_RRW_CNS)">{_idIns,en} {_idReg1,en}, {_idLargeCns,d}</DisplayString>
+    <DisplayString Condition="(_idInsFmt == IF_RRW_SHF) &amp;&amp; (_idLargeCns != 0)">{_idIns,en} {_idReg1,en}, {_idLargeCns,d}</DisplayString>
+    <DisplayString Condition="(_idInsFmt == IF_RRW_SHF) &amp;&amp; (_idLargeCns == 0)">{_idIns,en} {_idReg1,en}, {_idSmallCns,d}</DisplayString>
+    <DisplayString>{_idIns,en}</DisplayString>
+  </Type>
+
   <!-- utils -->
    <Type Name="jitstd::list&lt;*&gt;">
     <DisplayString Condition="m_nSize > 0">Size={m_nSize}</DisplayString>

src/coreclr/jit/codegenxarch.cpp

@@ -6035,8 +6035,7 @@ void CodeGen::genCompareInt(GenTree* treeNode)
     // TYP_UINT and TYP_ULONG should not appear here, only small types can be unsigned
     assert(!varTypeIsUnsigned(type) || varTypeIsSmall(type));
 
-    bool needsOCFlags = !tree->OperIs(GT_EQ, GT_NE);
-    if (canReuseFlags && emit->AreFlagsSetToZeroCmp(op1->GetRegNum(), emitTypeSize(type), needsOCFlags))
+    if (canReuseFlags && emit->AreFlagsSetToZeroCmp(op1->GetRegNum(), emitTypeSize(type), tree->OperGet()))
     {
         JITDUMP("Not emitting compare due to flags being already set\n");
     }

src/coreclr/jit/emitxarch.cpp

@@ -144,6 +144,98 @@ bool emitter::IsDstSrcSrcAVXInstruction(instruction ins)
     return ((CodeGenInterface::instInfo[ins] & INS_Flags_IsDstSrcSrcAVXInstruction) != 0) && IsAVXInstruction(ins);
 }
 
+//------------------------------------------------------------------------
+// DoesWriteZeroFlag: check if the instruction write the
+//     ZF flag.
+//
+// Arguments:
+//    ins - instruction to test
+//
+// Return Value:
+//    true if instruction writes the ZF flag, false otherwise.
+//
+bool emitter::DoesWriteZeroFlag(instruction ins)
+{
+    return (CodeGenInterface::instInfo[ins] & INS_FLAGS_WritesZF) != 0;
+}
+
+//------------------------------------------------------------------------
+// DoesResetOverflowAndCarryFlags: check if the instruction resets the
+//     OF and CF flag to 0.
+//
+// Arguments:
+//    ins - instruction to test
+//
+// Return Value:
+//    true if instruction resets the OF and CF flag, false otherwise.
+//
+bool emitter::DoesResetOverflowAndCarryFlags(instruction ins)
+{
+    return (CodeGenInterface::instInfo[ins] & INS_FLAGS_Resets_CF_OF_Flags) != 0;
+}
+
+//------------------------------------------------------------------------
+// IsFlagsAlwaysModified: check if the instruction guarantee to modify any flags.
+//
+// Arguments:
+//    id - instruction to test
+//
+// Return Value:
+//    false, if instruction is guaranteed to not modify any flag.
+//    true, if instruction will modify some flag.
+//
+bool emitter::IsFlagsAlwaysModified(instrDesc* id)
+{
+    instruction ins = id->idIns();
+    insFormat   fmt = id->idInsFmt();
+
+    if (fmt == IF_RRW_SHF)
+    {
+        if (id->idIsLargeCns())
+        {
+            return true;
+        }
+        else if (id->idSmallCns() == 0)
+        {
+            switch (ins)
+            {
+                // If shift-amount for below instructions is 0, then flags are unaffected.
+                case INS_rcl_N:
+                case INS_rcr_N:
+                case INS_rol_N:
+                case INS_ror_N:
+                case INS_shl_N:
+                case INS_shr_N:
+                case INS_sar_N:
+                    return false;
+                default:
+                    return true;
+            }
+        }
+    }
+    else if (fmt == IF_RRW)
+    {
+        switch (ins)
+        {
+            // If shift-amount for below instructions is 0, then flags are unaffected.
+            // So, to be conservative, do not optimize if the instruction has register
+            // as the shift-amount operand.
+            case INS_rcl:
+            case INS_rcr:
+            case INS_rol:
+            case INS_ror:
+            case INS_shl:
+            case INS_shr:
+            case INS_sar:
+                return false;
+            default:
+                return true;
+        }
+    }
+
+    return true;
+}
+
 //------------------------------------------------------------------------
 // AreUpper32BitsZero: check if some previously emitted
 //     instruction set the upper 32 bits of reg to zero.
@@ -220,27 +312,29 @@ bool emitter::AreUpper32BitsZero(regNumber reg)
 //                       the same values as if there were a compare to 0
 //
 // Arguments:
-//    reg - register of interest
-//    opSize - size of register
-//    needsOCFlags - additionally check the overflow and carry flags
+//    reg     - register of interest
+//    opSize  - size of register
+//    treeOps - type of tree node operation
 //
 // Return Value:
 //    true if the previous instruction set the flags for reg
 //    false if not, or if we can't safely determine
 //
 // Notes:
 //    Currently only looks back one instruction.
-bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, bool needsOCFlags)
+bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, genTreeOps treeOps)
 {
     assert(reg != REG_NA);
+
     // Don't look back across IG boundaries (possible control flow)
     if (emitCurIGinsCnt == 0 && ((emitCurIG->igFlags & IGF_EXTEND) == 0))
     {
         return false;
     }
 
-    instrDesc* id  = emitLastIns;
-    insFormat  fmt = id->idInsFmt();
+    instrDesc*  id      = emitLastIns;
+    instruction lastIns = id->idIns();
+    insFormat   fmt     = id->idInsFmt();
 
     // make sure op1 is a reg
     switch (fmt)
@@ -259,7 +353,6 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, bool needsOCF
         case IF_RRD:
         case IF_RRW:
             break;
-
         default:
             return false;
     }
@@ -269,34 +362,20 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, bool needsOCF
         return false;
     }
 
-    switch (id->idIns())
+    // Certain instruction like and, or and xor modifies exactly same flags
+    // as "test" instruction.
+    // They reset OF and CF to 0 and modifies SF, ZF and PF.
+    if (DoesResetOverflowAndCarryFlags(lastIns))
     {
-        case INS_adc:
-        case INS_add:
-        case INS_dec:
-        case INS_dec_l:
-        case INS_inc:
-        case INS_inc_l:
-        case INS_neg:
-        case INS_shr_1:
-        case INS_shl_1:
-        case INS_sar_1:
-        case INS_sbb:
-        case INS_sub:
-        case INS_xadd:
-            if (needsOCFlags)
-            {
-                return false;
-            }
-            FALLTHROUGH;
-        // these always set OC to 0
-        case INS_and:
-        case INS_or:
-        case INS_xor:
-            return id->idOpSize() == opSize;
+        return id->idOpSize() == opSize;
+    }
 
-        default:
-            break;
+    if ((treeOps == GT_EQ) || (treeOps == GT_NE))
+    {
+        if (DoesWriteZeroFlag(lastIns) && IsFlagsAlwaysModified(id))
+        {
+            return id->idOpSize() == opSize;
+        }
     }
 
     return false;

src/coreclr/jit/emitxarch.h

@@ -98,7 +98,7 @@ static bool IsMovInstruction(instruction ins);
 
 bool AreUpper32BitsZero(regNumber reg);
 
-bool AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, bool needsOCFlags);
+bool AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, genTreeOps treeOps);
 
 bool hasRexPrefix(code_t code)
 {
@@ -171,6 +171,10 @@ void SetContains256bitAVX(bool value)
 
 bool IsDstDstSrcAVXInstruction(instruction ins);
 bool IsDstSrcSrcAVXInstruction(instruction ins);
+bool DoesWriteZeroFlag(instruction ins);
+bool DoesResetOverflowAndCarryFlags(instruction ins);
+bool IsFlagsAlwaysModified(instrDesc* id);
+
 bool IsThreeOperandAVXInstruction(instruction ins)
 {
     return (IsDstDstSrcAVXInstruction(ins) || IsDstSrcSrcAVXInstruction(ins));

src/coreclr/jit/instr.h

@@ -86,14 +86,48 @@ enum GCtype : unsigned
 };
 
 #if defined(TARGET_XARCH)
-enum insFlags: uint8_t
+enum insFlags : uint32_t
 {
-    INS_FLAGS_None = 0x00,
-    INS_FLAGS_ReadsFlags = 0x01,
-    INS_FLAGS_WritesFlags = 0x02,
-    INS_FLAGS_x87Instr = 0x04,
-    INS_Flags_IsDstDstSrcAVXInstruction = 0x08,
-    INS_Flags_IsDstSrcSrcAVXInstruction = 0x10,
+    INS_FLAGS_None = 0,
+
+    // Reads EFLAGS
+    INS_FLAGS_ReadsCF = 1 << 0,
+    INS_FLAGS_ReadsPF = 1 << 1,
+    INS_FLAGS_ReadsAF = 1 << 2,
+    INS_FLAGS_ReadsZF = 1 << 3,
+    INS_FLAGS_ReadsSF = 1 << 4,
+    INS_FLAGS_ReadsDF = 1 << 5,
+    INS_FLAGS_ReadsOF = 1 << 6,
+    INS_FLAGS_ReadsAllFlagsExceptAF = INS_FLAGS_ReadsCF | INS_FLAGS_ReadsPF | INS_FLAGS_ReadsZF | INS_FLAGS_ReadsSF | INS_FLAGS_ReadsOF,
+    INS_FLAGS_Reads_CF_ZF_Flags = INS_FLAGS_ReadsCF | INS_FLAGS_ReadsZF,
+    INS_FLAGS_Reads_OF_SF_Flags = INS_FLAGS_ReadsOF | INS_FLAGS_ReadsSF,
+    INS_FLAGS_Reads_OF_SF_ZF_Flags = INS_FLAGS_ReadsOF | INS_FLAGS_ReadsSF | INS_FLAGS_ReadsZF,
+    INS_FLAGS_ReadsAllFlags = INS_FLAGS_ReadsAF | INS_FLAGS_ReadsAllFlagsExceptAF,
+
+    // Writes EFLAGS
+    INS_FLAGS_WritesCF = 1 << 7,
+    INS_FLAGS_WritesPF = 1 << 8,
+    INS_FLAGS_WritesAF = 1 << 9,
+    INS_FLAGS_WritesZF = 1 << 10,
+    INS_FLAGS_WritesSF = 1 << 11,
+    INS_FLAGS_WritesDF = 1 << 12,
+    INS_FLAGS_WritesOF = 1 << 13,
+    INS_FLAGS_WritesAllFlagsExceptCF = INS_FLAGS_WritesPF | INS_FLAGS_WritesAF | INS_FLAGS_WritesZF | INS_FLAGS_WritesSF | INS_FLAGS_WritesOF,
+    INS_FLAGS_WritesAllFlagsExceptOF = INS_FLAGS_WritesCF | INS_FLAGS_WritesPF | INS_FLAGS_WritesAF | INS_FLAGS_WritesZF | INS_FLAGS_WritesSF,
+    INS_FLAGS_WritesAllFlags = INS_FLAGS_WritesCF | INS_FLAGS_WritesAllFlagsExceptCF,
+
+    // Resets EFLAGS
+    INS_FLAGS_Resets_OF_Flags       = 1 << 14,
+    INS_FLAGS_Resets_CF_OF_Flags    = 1 << 15,
+    INS_FLAGS_Resets_OF_SF_PF_Flags = 1 << 16,
+    INS_FLAGS_ResetsAllFlagsExceptZF = 1 << 17,
+
+    // x87 instruction
+    INS_FLAGS_x87Instr = 1 << 18,
+
+    // Avx
+    INS_Flags_IsDstDstSrcAVXInstruction = 1 << 19,
+    INS_Flags_IsDstSrcSrcAVXInstruction = 1 << 20,
 
     //  TODO-Cleanup:  Remove this flag and its usage from TARGET_XARCH
     INS_FLAGS_DONT_CARE = 0x00,