Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Eliminate redundant test instruction #53214

Merged
merged 10 commits into from
Jun 5, 2021
Merged

Eliminate redundant test instruction #53214

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
06dda15
Correctly track how x86 instructions read/write flags
tannergooding May 21, 2021
b6f80ea
For GT_EQ/GT_NE, reuse flag
kunalspathak May 24, 2021
8867374
Explicit flags for jcc, setcc, comvcc
kunalspathak May 25, 2021
01fb6ba
Add reset flags
kunalspathak May 25, 2021
81eb9eb
remove duplicate enum
kunalspathak May 28, 2021
53c4325
Handle cases where shift-amount is 0
kunalspathak Jun 1, 2021
7ded469
Add helper method for Resets OF/CF flags
kunalspathak Jun 1, 2021
e7e04b7
Rename methods
kunalspathak Jun 2, 2021
b1e0fc3
one more rename
kunalspathak Jun 2, 2021
068fb15
review feedback
kunalspathak Jun 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/coreclr/jit/clrjit.natvis
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
kunalspathak marked this conversation as resolved.
Show resolved Hide resolved
</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
Open in desktop
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
135 changes: 105 additions & 30 deletions src/coreclr/jit/emitxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,97 @@ bool emitter::IsDstSrcSrcAVXInstruction(instruction ins)
return ((CodeGenInterface::instInfo[ins] & INS_Flags_IsDstSrcSrcAVXInstruction) != 0) && IsAVXInstruction(ins);
}

//------------------------------------------------------------------------
// DoesWritesZeroFlag: check if the instruction write the
kunalspathak marked this conversation as resolved.
Show resolved Hide resolved
// ZF flag.
//
// Arguments:
// ins - instruction to test
//
// Return Value:
// true if instruction writes the ZF flag, false otherwise.
//
bool emitter::DoesWritesZeroFlag(instruction ins)
{
return (CodeGenInterface::instInfo[ins] & INS_FLAGS_WritesZF) != 0;
}

//------------------------------------------------------------------------
// DoesResetsOverflowAndCarryFlags: check if the instruction resets the
kunalspathak marked this conversation as resolved.
Show resolved Hide resolved
// OF and CF flag.
//
// Arguments:
// ins - instruction to test
//
// Return Value:
// true if instruction resets the OF and CF flag, false otherwise.
kunalspathak marked this conversation as resolved.
Show resolved Hide resolved
//
bool emitter::DoesResetsOverflowAndCarryFlags(instruction ins)
{
return (CodeGenInterface::instInfo[ins] & INS_FLAGS_Resets_CF_OF_Flags) != 0;
}

//------------------------------------------------------------------------
// IsFlagsAlwaysModified: check if the instruction always modifies the flags.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// IsFlagsAlwaysModified: check if the instruction always modifies the flags.
// AreFlagsAlwaysModified: check if the instruction always modifies the flags.

I am a bit confused on what this is supposed to check. Does it mean: return true if there is at least one flag bit that is always modified by this instruction?

Seems odd we would ask that question instead of asking whether some specific set of flags is always modified.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For shift instructions, if a shift-amount is 0, it keeps the flags unmodified. For them, we shouldn't rely on last instruction, but the flags might have set by one of the previous instructions. As such, this method returns false saying that these instructions (under certain circumstances) guaranteed to not modify flags and the caller shouldn't rely on last instruction to eliminate test. For remaining instructions, they modify some flags and can eliminate test safely. I will add more explicit comment.

See my comment #53214 (comment) when I was not handling this case.

//
// Arguments:
// id - instruction to test
//
// Return Value:
// true if instruction always modified any flag, false otherwise.
//
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;
Copy link
Member

@tannergooding tannergooding Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might require some careful considerations.

For the purposes of this PR, this is fine and is simply used as a "did the previous instruction set the flag and therefore a test can be skipped" check.

However, for future improvements we may consider more advanced analysis such as:

if (!IsFlagsModified(prevInstruction))
{
   // can depend on prevInstruction - 1 state being valid 
}

Then, this will become problematic. The latter happens in Clang/MSVC in various places and are one of the reasons that mulx, rorx, sarx, shlx, and shrx are exposed on modern hardware.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can return some ternary state here representing "yes", "no", and "it depends"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today, we only track prevInstruction (emitLastIns). The way I read your comment, we will need to have access to last couple of instructions (if not more) to make sure the state is valid?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it's not something I think we need to be concerned with "today", but rather something I could see being problematic in the future for certain optimizations.

The method implies boolean state but in practice its slightly more nuanced and one of the answers is "maybe", so it may end up confusing callers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method implies boolean state but in practice its slightly more nuanced and one of the answers is "maybe", so it may end up confusing callers.

Exactly - what are callers supposed to do with "maybe". So for now, I will leave it as-is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is contextual. In the case of this PR, you care "did the last instruction definitely set the flag I care about" and so you would treat maybe as false.

In other cases, optimizations may care "did the last instruction definitely not set the flag I care about" and so they would treat it as true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed the method to IsFlagsAlwaysModified.

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 @@ -230,17 +321,19 @@ bool emitter::AreUpper32BitsZero(regNumber reg)
//
// 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)
kunalspathak marked this conversation as resolved.
Show resolved Hide resolved
{
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 @@ -259,7 +352,6 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, bool needsOCF
case IF_RRD:
case IF_RRW:
break;

default:
return false;
}
Expand All @@ -269,34 +361,17 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, bool needsOCF
return false;
}

switch (id->idIns())
if (DoesResetsOverflowAndCarryFlags(lastIns))
kunalspathak marked this conversation as resolved.
Show resolved Hide resolved
{
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 (DoesWritesZeroFlag(lastIns) && IsFlagsAlwaysModified(id))
{
return id->idOpSize() == opSize;
}
}

return false;
Expand Down
6 changes: 5 additions & 1 deletion src/coreclr/jit/emitxarch.h
View file
Open in desktop
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 DoesWritesZeroFlag(instruction ins);
bool DoesResetsOverflowAndCarryFlags(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
Open in desktop
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