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.

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

XARCH: Remove redudant tests for GT_LT/GT_GE relops. #61152

Merged
merged 2 commits into from
Nov 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions src/coreclr/jit/codegenlinear.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2618,6 +2618,16 @@ void CodeGen::genCodeForJumpTrue(GenTreeOp* jtrue)

condition = GenCondition(GenCondition::P);
}

if (relop->MarkedForSignJumpOpt())
{
// If relop was previously marked for a signed jump check optimization because of SF flag
// reuse, replace jge/jl with jns/js.

assert(relop->OperGet() == GT_LT || relop->OperGet() == GT_GE);
condition = (relop->OperGet() == GT_LT) ? GenCondition(GenCondition::S) : GenCondition(GenCondition::NS);
}

#endif

inst_JCC(condition, compiler->compCurBB->bbJumpDest);
Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/jit/codegenxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6227,11 +6227,18 @@ void CodeGen::genCompareInt(GenTree* treeNode)
assert(genTypeSize(type) <= genTypeSize(TYP_I_IMPL));
// TYP_UINT and TYP_ULONG should not appear here, only small types can be unsigned
assert(!varTypeIsUnsigned(type) || varTypeIsSmall(type));
// Sign jump optimization should only be set the following check
assert((tree->gtFlags & GTF_RELOP_SJUMP_OPT) == 0);

if (canReuseFlags && emit->AreFlagsSetToZeroCmp(op1->GetRegNum(), emitTypeSize(type), tree->OperGet()))
{
JITDUMP("Not emitting compare due to flags being already set\n");
}
else if (canReuseFlags && emit->AreFlagsSetForSignJumpOpt(op1->GetRegNum(), emitTypeSize(type), tree))
{
JITDUMP("Not emitting compare due to sign being already set, follow up instr will transform jump\n");
tree->gtFlags |= GTF_RELOP_SJUMP_OPT;
Copy link
Member

Choose a reason for hiding this comment

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

Can you either always clear this new flag up above before the if on line 6231, or assert there that it's not set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the assertion next to the other ones. Please let me know if the comment needs to change or if its good enough.

}
else
{
emit->emitInsBinary(ins, emitTypeSize(type), op1, op2);
Expand Down
93 changes: 93 additions & 0 deletions src/coreclr/jit/emitxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,21 @@ bool emitter::DoesWriteZeroFlag(instruction ins)
return (CodeGenInterface::instInfo[ins] & Writes_ZF) != 0;
}

//------------------------------------------------------------------------
// DoesWriteSignFlag: check if the instruction writes the
// SF flag.
//
// Arguments:
// ins - instruction to test
//
// Return Value:
// true if instruction writes the SF flag, false otherwise.
//
bool emitter::DoesWriteSignFlag(instruction ins)
{
return (CodeGenInterface::instInfo[ins] & Writes_SF) != 0;
}

//------------------------------------------------------------------------
// DoesResetOverflowAndCarryFlags: check if the instruction resets the
// OF and CF flag to 0.
Expand Down Expand Up @@ -338,6 +353,11 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, genTreeOps tr
{
assert(reg != REG_NA);

if (!emitComp->opts.OptimizationEnabled())
{
return false;
}

// Don't look back across IG boundaries (possible control flow)
if (emitCurIGinsCnt == 0 && ((emitCurIG->igFlags & IGF_EXTEND) == 0))
{
Expand Down Expand Up @@ -393,6 +413,79 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, genTreeOps tr
return false;
}

//------------------------------------------------------------------------
// AreFlagsSetToForSignJumpOpt: checks if the previous instruction set the SF if the tree
// node qualifies for a jg/jle to jns/js optimization
//
// Arguments:
// reg - register of interest
// opSize - size of register
// relop - relational tree node
//
// Return Value:
// true if the tree node qualifies for the jg/jle to jns/js optimization
// false if not, or if we can't safely determine
//
// Notes:
// Currently only looks back one instruction.
bool emitter::AreFlagsSetForSignJumpOpt(regNumber reg, emitAttr opSize, GenTree* relop)
{
assert(reg != REG_NA);

if (!emitComp->opts.OptimizationEnabled())
{
return false;
}

// Don't look back across IG boundaries (possible control flow)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a check like

   if (!emitComp->opts.OptimizationEnabled()) 
   {
      return false;
   }

(or else assert we're optimizing)

I realize this is also guaranteed by the way the current caller sets canReuseFlags but would prefer we check these things closer to where we actually do the transformation.

(and add similar to AreFlagsSetToZeroCmp if you don't mind)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in both spots. Thanks!

if (emitCurIGinsCnt == 0 && ((emitCurIG->igFlags & IGF_EXTEND) == 0))
Copy link
Member

Choose a reason for hiding this comment

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

The check emitCurIGinsCnt == 0 && ((emitCurIG->igFlags & IGF_EXTEND) == 0) is becoming more and more popular (we already do it at 6 different places), can you extract this in a method?

Copy link
Member

Choose a reason for hiding this comment

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

I am ok to have a follow-up PR as Andy pointed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can do this with a follow-up PR. Happy to help.

{
return false;
}

instrDesc* id = emitLastIns;
instruction lastIns = id->idIns();
insFormat fmt = id->idInsFmt();

// make sure op1 is a reg
switch (fmt)
{
case IF_RWR_CNS:
case IF_RRW_CNS:
case IF_RRW_SHF:
case IF_RWR_RRD:
case IF_RRW_RRD:
case IF_RWR_MRD:
case IF_RWR_SRD:
case IF_RRW_SRD:
case IF_RWR_ARD:
case IF_RRW_ARD:
case IF_RWR:
case IF_RRD:
case IF_RRW:
break;
default:
return false;
}

if (id->idReg1() != reg)
{
return false;
}
Comment on lines +448 to +474
Copy link
Member

Choose a reason for hiding this comment

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

We should think about extracting things that are common as helper methods. But maybe do this as a zero-diff follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, happy to do a follow up refactor.


// If we have a GT_GE/GT_LT which generates an jge/jl, and the previous instruction
// sets the SF, we can omit a test instruction and check for jns/js.
if ((relop->OperGet() == GT_GE || relop->OperGet() == GT_LT) && !GenCondition::FromRelop(relop).IsUnsigned())
{
if (DoesWriteSignFlag(lastIns) && IsFlagsAlwaysModified(id))
{
return id->idOpSize() == opSize;
}
}

return false;
}

//------------------------------------------------------------------------
// IsDstSrcImmAvxInstruction: Checks if the instruction has a "reg, reg/mem, imm" or
// "reg/mem, reg, imm" form for the legacy, VEX, and EVEX
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/jit/emitxarch.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ static bool IsJmpInstruction(instruction ins);
bool AreUpper32BitsZero(regNumber reg);

bool AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, genTreeOps treeOps);
bool AreFlagsSetForSignJumpOpt(regNumber reg, emitAttr opSize, GenTree* tree);

bool hasRexPrefix(code_t code)
{
Expand Down Expand Up @@ -190,6 +191,7 @@ void SetContains256bitAVX(bool value)
bool IsDstDstSrcAVXInstruction(instruction ins);
bool IsDstSrcSrcAVXInstruction(instruction ins);
bool DoesWriteZeroFlag(instruction ins);
bool DoesWriteSignFlag(instruction ins);
bool DoesResetOverflowAndCarryFlags(instruction ins);
bool IsFlagsAlwaysModified(instrDesc* id);

Expand Down
9 changes: 9 additions & 0 deletions src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,8 @@ enum GenTreeFlags : unsigned int
GTF_RELOP_QMARK = 0x20000000, // GT_<relop> -- the node is the condition for ?:
GTF_RELOP_ZTT = 0x08000000, // GT_<relop> -- Loop test cloned for converting while-loops into do-while
// with explicit "loop test" in the header block.
GTF_RELOP_SJUMP_OPT = 0x04000000, // GT_<relop> -- Swap signed jl/jge with js/jns during emitter, reuses flags
// from previous instruction.

GTF_JCMP_EQ = 0x80000000, // GTF_JCMP_EQ -- Branch on equal rather than not equal
GTF_JCMP_TST = 0x40000000, // GTF_JCMP_TST -- Use bit test instruction rather than compare against zero instruction
Expand Down Expand Up @@ -3027,6 +3029,13 @@ struct GenTreeOp : public GenTreeUnOp
{
}
#endif

// True if this relop is marked for a transform during the emitter
// phase, e.g., jge => jns
bool MarkedForSignJumpOpt() const
{
return (gtFlags & GTF_RELOP_SJUMP_OPT) != 0;
}
};

struct GenTreeVal : public GenTree
Expand Down