Skip to content

Commit

Permalink
Eliminate redundant test instruction (#53214)
Browse files Browse the repository at this point in the history 
* Correctly track how x86 instructions read/write flags

* For GT_EQ/GT_NE, reuse flag

* Explicit flags for jcc, setcc, comvcc

* Add reset flags

* remove duplicate enum

* Handle cases where shift-amount is 0

* Add helper method for Resets OF/CF flags

* Rename methods

* one more rename

* review feedback

Co-authored-by: Tanner Gooding <tagoo@outlook.com>
  • Loading branch information
@kunalspathak @tannergooding
kunalspathak and tannergooding committed Jun 5, 2021
1 parent 9c3a78d commit 9f92b9a
Show file tree
Hide file tree
Showing 6 changed files with 322 additions and 198 deletions.
8 changes: 8 additions & 0 deletions src/coreclr/jit/clrjit.natvis
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,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>
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/codegenxarch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
145 changes: 112 additions & 33 deletions src/coreclr/jit/emitxarch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,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.
Expand Down Expand Up @@ -225,27 +317,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)
Expand All @@ -264,7 +358,6 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, bool needsOCF
case IF_RRD:
case IF_RRW:
break;

default:
return false;
}
Expand All @@ -274,34 +367,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;
Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/jit/emitxarch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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));
Expand Down
48 changes: 41 additions & 7 deletions src/coreclr/jit/instr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 9f92b9a

Please sign in to comment.