From 3bbd122c8eac0f7de1460b2d9e1ea9fa697cf0f7 Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Mon, 29 Jun 2020 13:29:01 -0400 Subject: [PATCH 1/7] Remove some unnecessary tests --- src/coreclr/src/jit/codegenxarch.cpp | 27 +++++++++---- src/coreclr/src/jit/emitxarch.cpp | 59 ++++++++++++++++++++++++++++ src/coreclr/src/jit/emitxarch.h | 2 + 3 files changed, 81 insertions(+), 7 deletions(-) diff --git a/src/coreclr/src/jit/codegenxarch.cpp b/src/coreclr/src/jit/codegenxarch.cpp index 48b43f58ea72ce..c51e1d3ccea5fc 100644 --- a/src/coreclr/src/jit/codegenxarch.cpp +++ b/src/coreclr/src/jit/codegenxarch.cpp @@ -5908,12 +5908,14 @@ void CodeGen::genCompareInt(GenTree* treeNode) { assert(treeNode->OperIsCompare() || treeNode->OperIs(GT_CMP)); - GenTreeOp* tree = treeNode->AsOp(); - GenTree* op1 = tree->gtOp1; - GenTree* op2 = tree->gtOp2; - var_types op1Type = op1->TypeGet(); - var_types op2Type = op2->TypeGet(); - regNumber targetReg = tree->GetRegNum(); + GenTreeOp* tree = treeNode->AsOp(); + GenTree* op1 = tree->gtOp1; + GenTree* op2 = tree->gtOp2; + var_types op1Type = op1->TypeGet(); + var_types op2Type = op2->TypeGet(); + regNumber targetReg = tree->GetRegNum(); + emitter* emit = GetEmitter(); + bool canReuseZeroFlag = false; genConsumeOperands(tree); @@ -5945,6 +5947,10 @@ void CodeGen::genCompareInt(GenTree* treeNode) } else if (op1->isUsedFromReg() && op2->IsIntegralConst(0)) { + if ((treeNode->OperIs(GT_EQ) || treeNode->OperIs(GT_NE)) && compiler->opts.OptimizationEnabled()) + { + canReuseZeroFlag = true; + } // We're comparing a register to 0 so we can generate "test reg1, reg1" // instead of the longer "cmp reg1, 0" ins = INS_test; @@ -5995,7 +6001,14 @@ 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)); - GetEmitter()->emitInsBinary(ins, emitTypeSize(type), op1, op2); + if (canReuseZeroFlag && emit->IsZeroFlagSet(op1->GetRegNum(), emitTypeSize(type))) + { + JITDUMP("Not emitting compare due to flags being already set\n"); + } + else + { + emit->emitInsBinary(ins, emitTypeSize(type), op1, op2); + } // Are we evaluating this into a register? if (targetReg != REG_NA) diff --git a/src/coreclr/src/jit/emitxarch.cpp b/src/coreclr/src/jit/emitxarch.cpp index e1f7f8ec0be5ed..5bf447d1291e27 100644 --- a/src/coreclr/src/jit/emitxarch.cpp +++ b/src/coreclr/src/jit/emitxarch.cpp @@ -214,6 +214,65 @@ bool emitter::AreUpper32BitsZero(regNumber reg) return false; } +//------------------------------------------------------------------------ +// IsZeroFlagSet: Checks if the previous instruction set the zero flag for +// reg of opSize size to zero. +// +// Arguments: +// reg - register of interest +// opSize - size of register +// +// Return Value: +// true if the previous instruction set the zero flag for reg +// false if not, or if we can't safely determine +// +// Notes: +// Currently only looks back one instruction. +bool emitter::IsZeroFlagSet(regNumber reg, emitAttr opSize) +{ + // Don't look back across IG boundaries (possible control flow) + if (emitCurIGinsCnt == 0) + { + return false; + } + else if (reg == REG_NA) + { + return false; + } + + instrDesc* id = emitLastIns; + + if (id->idReg1() != reg) + { + return false; + } + + switch (id->idIns()) + { + case INS_adc: + case INS_add: + case INS_and: + case INS_dec: + case INS_dec_l: + case INS_inc: + case INS_inc_l: + case INS_neg: + case INS_or: + case INS_shl_1: + case INS_sar_1: + case INS_sbb: + case INS_sub: + case INS_xadd: + case INS_xor: + return id->idOpSize() == opSize; + + default: + break; + } + + 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 diff --git a/src/coreclr/src/jit/emitxarch.h b/src/coreclr/src/jit/emitxarch.h index 9c380e1451c342..34bdb3b76b5094 100644 --- a/src/coreclr/src/jit/emitxarch.h +++ b/src/coreclr/src/jit/emitxarch.h @@ -97,6 +97,8 @@ bool Is4ByteSSEInstruction(instruction ins); bool AreUpper32BitsZero(regNumber reg); +bool IsZeroFlagSet(regNumber reg, emitAttr opSize); + bool hasRexPrefix(code_t code) { #ifdef TARGET_AMD64 From f43739bb99d4f1b1b0b97671448e9692c528caba Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Tue, 30 Jun 2020 17:53:31 -0400 Subject: [PATCH 2/7] Handle more cases --- src/coreclr/src/jit/codegenxarch.cpp | 30 ++++++++++++++++------------ src/coreclr/src/jit/emitxarch.cpp | 27 ++++++++++++++----------- src/coreclr/src/jit/emitxarch.h | 2 +- 3 files changed, 33 insertions(+), 26 deletions(-) diff --git a/src/coreclr/src/jit/codegenxarch.cpp b/src/coreclr/src/jit/codegenxarch.cpp index c51e1d3ccea5fc..61d734dc0d762a 100644 --- a/src/coreclr/src/jit/codegenxarch.cpp +++ b/src/coreclr/src/jit/codegenxarch.cpp @@ -5908,14 +5908,14 @@ void CodeGen::genCompareInt(GenTree* treeNode) { assert(treeNode->OperIsCompare() || treeNode->OperIs(GT_CMP)); - GenTreeOp* tree = treeNode->AsOp(); - GenTree* op1 = tree->gtOp1; - GenTree* op2 = tree->gtOp2; - var_types op1Type = op1->TypeGet(); - var_types op2Type = op2->TypeGet(); - regNumber targetReg = tree->GetRegNum(); - emitter* emit = GetEmitter(); - bool canReuseZeroFlag = false; + GenTreeOp* tree = treeNode->AsOp(); + GenTree* op1 = tree->gtOp1; + GenTree* op2 = tree->gtOp2; + var_types op1Type = op1->TypeGet(); + var_types op2Type = op2->TypeGet(); + regNumber targetReg = tree->GetRegNum(); + emitter* emit = GetEmitter(); + bool canReuseSZFlags = false; genConsumeOperands(tree); @@ -5925,6 +5925,12 @@ void CodeGen::genCompareInt(GenTree* treeNode) instruction ins; var_types type = TYP_UNKNOWN; + if (op1->isUsedFromReg() && op2->IsIntegralConst(0) && !treeNode->OperIs(GT_CMP) && + compiler->opts.OptimizationEnabled()) + { + canReuseSZFlags = true; + } + if (tree->OperIs(GT_TEST_EQ, GT_TEST_NE)) { ins = INS_test; @@ -5947,10 +5953,6 @@ void CodeGen::genCompareInt(GenTree* treeNode) } else if (op1->isUsedFromReg() && op2->IsIntegralConst(0)) { - if ((treeNode->OperIs(GT_EQ) || treeNode->OperIs(GT_NE)) && compiler->opts.OptimizationEnabled()) - { - canReuseZeroFlag = true; - } // We're comparing a register to 0 so we can generate "test reg1, reg1" // instead of the longer "cmp reg1, 0" ins = INS_test; @@ -6001,7 +6003,9 @@ 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)); - if (canReuseZeroFlag && emit->IsZeroFlagSet(op1->GetRegNum(), emitTypeSize(type))) + if (canReuseSZFlags && + emit->AreSZOFlagsSet(op1->GetRegNum(), emitTypeSize(type), + tree->IsUnsigned() && !treeNode->OperIs(GT_EQ, GT_NE))) { JITDUMP("Not emitting compare due to flags being already set\n"); } diff --git a/src/coreclr/src/jit/emitxarch.cpp b/src/coreclr/src/jit/emitxarch.cpp index 5bf447d1291e27..3b89a579956c5d 100644 --- a/src/coreclr/src/jit/emitxarch.cpp +++ b/src/coreclr/src/jit/emitxarch.cpp @@ -215,27 +215,25 @@ bool emitter::AreUpper32BitsZero(regNumber reg) } //------------------------------------------------------------------------ -// IsZeroFlagSet: Checks if the previous instruction set the zero flag for -// reg of opSize size to zero. +// IsZeroFlagSet: Checks if the previous instruction set the SZO flags and optionally CF for +// reg of opSize size // // Arguments: // reg - register of interest // opSize - size of register +// needsCF - also check the carry flag // // Return Value: -// true if the previous instruction set the zero flag for reg +// 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::IsZeroFlagSet(regNumber reg, emitAttr opSize) +bool emitter::AreSZOFlagsSet(regNumber reg, emitAttr opSize, bool needsCF) { + assert(reg != REG_NA); // Don't look back across IG boundaries (possible control flow) - if (emitCurIGinsCnt == 0) - { - return false; - } - else if (reg == REG_NA) + if (emitCurIGinsCnt == 0 && ((emitCurIG->igFlags & IGF_EXTEND) == 0)) { return false; } @@ -249,15 +247,20 @@ bool emitter::IsZeroFlagSet(regNumber reg, emitAttr opSize) switch (id->idIns()) { - case INS_adc: - case INS_add: - case INS_and: case INS_dec: case INS_dec_l: case INS_inc: case INS_inc_l: + if (needsCF) + { + return false; + } + case INS_adc: + case INS_add: + case INS_and: case INS_neg: case INS_or: + case INS_shr_1: case INS_shl_1: case INS_sar_1: case INS_sbb: diff --git a/src/coreclr/src/jit/emitxarch.h b/src/coreclr/src/jit/emitxarch.h index 34bdb3b76b5094..9ef0a88ba479a4 100644 --- a/src/coreclr/src/jit/emitxarch.h +++ b/src/coreclr/src/jit/emitxarch.h @@ -97,7 +97,7 @@ bool Is4ByteSSEInstruction(instruction ins); bool AreUpper32BitsZero(regNumber reg); -bool IsZeroFlagSet(regNumber reg, emitAttr opSize); +bool AreSZOFlagsSet(regNumber reg, emitAttr opSize, bool needsCF); bool hasRexPrefix(code_t code) { From e8aa02beaf375a75342073af3552da246e259c78 Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Tue, 30 Jun 2020 20:52:46 -0400 Subject: [PATCH 3/7] Fix some bugs --- src/coreclr/src/jit/codegenxarch.cpp | 33 ++++++++++--------- src/coreclr/src/jit/emitxarch.cpp | 48 ++++++++++++++++++++-------- src/coreclr/src/jit/emitxarch.h | 2 +- 3 files changed, 52 insertions(+), 31 deletions(-) diff --git a/src/coreclr/src/jit/codegenxarch.cpp b/src/coreclr/src/jit/codegenxarch.cpp index 61d734dc0d762a..da066ffd120f26 100644 --- a/src/coreclr/src/jit/codegenxarch.cpp +++ b/src/coreclr/src/jit/codegenxarch.cpp @@ -5908,14 +5908,14 @@ void CodeGen::genCompareInt(GenTree* treeNode) { assert(treeNode->OperIsCompare() || treeNode->OperIs(GT_CMP)); - GenTreeOp* tree = treeNode->AsOp(); - GenTree* op1 = tree->gtOp1; - GenTree* op2 = tree->gtOp2; - var_types op1Type = op1->TypeGet(); - var_types op2Type = op2->TypeGet(); - regNumber targetReg = tree->GetRegNum(); - emitter* emit = GetEmitter(); - bool canReuseSZFlags = false; + GenTreeOp* tree = treeNode->AsOp(); + GenTree* op1 = tree->gtOp1; + GenTree* op2 = tree->gtOp2; + var_types op1Type = op1->TypeGet(); + var_types op2Type = op2->TypeGet(); + regNumber targetReg = tree->GetRegNum(); + emitter* emit = GetEmitter(); + bool canReuseFlags = false; genConsumeOperands(tree); @@ -5925,12 +5925,6 @@ void CodeGen::genCompareInt(GenTree* treeNode) instruction ins; var_types type = TYP_UNKNOWN; - if (op1->isUsedFromReg() && op2->IsIntegralConst(0) && !treeNode->OperIs(GT_CMP) && - compiler->opts.OptimizationEnabled()) - { - canReuseSZFlags = true; - } - if (tree->OperIs(GT_TEST_EQ, GT_TEST_NE)) { ins = INS_test; @@ -5953,6 +5947,11 @@ void CodeGen::genCompareInt(GenTree* treeNode) } else if (op1->isUsedFromReg() && op2->IsIntegralConst(0)) { + if (compiler->opts.OptimizationEnabled()) + { + canReuseFlags = true; + } + // We're comparing a register to 0 so we can generate "test reg1, reg1" // instead of the longer "cmp reg1, 0" ins = INS_test; @@ -6003,9 +6002,9 @@ 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)); - if (canReuseSZFlags && - emit->AreSZOFlagsSet(op1->GetRegNum(), emitTypeSize(type), - tree->IsUnsigned() && !treeNode->OperIs(GT_EQ, GT_NE))) + if (canReuseFlags && + emit->AreFlagsSetToZeroCmp(op1->GetRegNum(), emitTypeSize(type), + tree->OperIs(GT_CMP) || !tree->OperIs(GT_EQ, GT_NE))) { JITDUMP("Not emitting compare due to flags being already set\n"); } diff --git a/src/coreclr/src/jit/emitxarch.cpp b/src/coreclr/src/jit/emitxarch.cpp index 4bafe2323d124b..41e90ddfdd1c41 100644 --- a/src/coreclr/src/jit/emitxarch.cpp +++ b/src/coreclr/src/jit/emitxarch.cpp @@ -215,13 +215,13 @@ bool emitter::AreUpper32BitsZero(regNumber reg) } //------------------------------------------------------------------------ -// IsZeroFlagSet: Checks if the previous instruction set the SZO flags and optionally CF for -// reg of opSize size +// AreFlagsSetToZeroCmp: Checks if the previous instruction set the SZ, and optionally OC, flags to +// the the same values as if there were a compare to 0 // // Arguments: // reg - register of interest // opSize - size of register -// needsCF - also check the carry flag +// needsOCFlags - additionally check the carry and overflow flag // // Return Value: // true if the previous instruction set the flags for reg @@ -229,7 +229,7 @@ bool emitter::AreUpper32BitsZero(regNumber reg) // // Notes: // Currently only looks back one instruction. -bool emitter::AreSZOFlagsSet(regNumber reg, emitAttr opSize, bool needsCF) +bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, bool needsOCFlags) { assert(reg != REG_NA); // Don't look back across IG boundaries (possible control flow) @@ -238,7 +238,29 @@ bool emitter::AreSZOFlagsSet(regNumber reg, emitAttr opSize, bool needsCF) return false; } - instrDesc* id = emitLastIns; + instrDesc* id = emitLastIns; + insFormat fmt = id->idInsFmt(); + + 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) { @@ -247,25 +269,25 @@ bool emitter::AreSZOFlagsSet(regNumber reg, emitAttr opSize, bool needsCF) switch (id->idIns()) { + case INS_adc: + case INS_add: case INS_dec: case INS_dec_l: case INS_inc: case INS_inc_l: - if (needsCF) - { - return false; - } - case INS_adc: - case INS_add: - case INS_and: case INS_neg: - case INS_or: 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; + } + case INS_and: + case INS_or: case INS_xor: return id->idOpSize() == opSize; diff --git a/src/coreclr/src/jit/emitxarch.h b/src/coreclr/src/jit/emitxarch.h index 9ef0a88ba479a4..51fd26eeecf35a 100644 --- a/src/coreclr/src/jit/emitxarch.h +++ b/src/coreclr/src/jit/emitxarch.h @@ -97,7 +97,7 @@ bool Is4ByteSSEInstruction(instruction ins); bool AreUpper32BitsZero(regNumber reg); -bool AreSZOFlagsSet(regNumber reg, emitAttr opSize, bool needsCF); +bool AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, bool needsOCFlags); bool hasRexPrefix(code_t code) { From c30822b537f685047a7de89fa4e815bc994490aa Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Fri, 3 Jul 2020 21:06:43 -0400 Subject: [PATCH 4/7] Fix some comments --- src/coreclr/src/jit/emitxarch.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/jit/emitxarch.cpp b/src/coreclr/src/jit/emitxarch.cpp index 41e90ddfdd1c41..8db34c4ee0e4aa 100644 --- a/src/coreclr/src/jit/emitxarch.cpp +++ b/src/coreclr/src/jit/emitxarch.cpp @@ -216,12 +216,12 @@ bool emitter::AreUpper32BitsZero(regNumber reg) //------------------------------------------------------------------------ // AreFlagsSetToZeroCmp: Checks if the previous instruction set the SZ, and optionally OC, flags to -// the the same values as if there were a compare to 0 +// the same values as if there were a compare to 0 // // Arguments: // reg - register of interest // opSize - size of register -// needsOCFlags - additionally check the carry and overflow flag +// needsOCFlags - additionally check the overflow and carry flags // // Return Value: // true if the previous instruction set the flags for reg From 571bf9235bc364925419d354a67e673c119e191f Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Mon, 13 Jul 2020 18:16:54 -0400 Subject: [PATCH 5/7] fix up docs some --- src/coreclr/src/jit/codegenxarch.cpp | 5 ++--- src/coreclr/src/jit/emitxarch.cpp | 2 ++ 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/coreclr/src/jit/codegenxarch.cpp b/src/coreclr/src/jit/codegenxarch.cpp index da066ffd120f26..7d377d07669111 100644 --- a/src/coreclr/src/jit/codegenxarch.cpp +++ b/src/coreclr/src/jit/codegenxarch.cpp @@ -6002,9 +6002,8 @@ 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)); - if (canReuseFlags && - emit->AreFlagsSetToZeroCmp(op1->GetRegNum(), emitTypeSize(type), - tree->OperIs(GT_CMP) || !tree->OperIs(GT_EQ, GT_NE))) + bool needsOCFlags = !tree->OperIs(GT_EQ, GT_NE); + if (canReuseFlags && emit->AreFlagsSetToZeroCmp(op1->GetRegNum(), emitTypeSize(type), needsOCFlags)) { JITDUMP("Not emitting compare due to flags being already set\n"); } diff --git a/src/coreclr/src/jit/emitxarch.cpp b/src/coreclr/src/jit/emitxarch.cpp index 8db34c4ee0e4aa..a71be5f2eb192a 100644 --- a/src/coreclr/src/jit/emitxarch.cpp +++ b/src/coreclr/src/jit/emitxarch.cpp @@ -241,6 +241,7 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, bool needsOCF instrDesc* id = emitLastIns; insFormat fmt = id->idInsFmt(); + //make sure op1 is a reg switch (fmt) { case IF_RWR_CNS: @@ -286,6 +287,7 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, bool needsOCF { return false; } + // these always set OC to 0 case INS_and: case INS_or: case INS_xor: From 1da9fb67b9fbc8f92b3a914b880cc2752c40ee34 Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Mon, 13 Jul 2020 19:24:24 -0400 Subject: [PATCH 6/7] add fallthrough --- src/coreclr/src/jit/emitxarch.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/src/jit/emitxarch.cpp b/src/coreclr/src/jit/emitxarch.cpp index a71be5f2eb192a..1d23ae36eebd7b 100644 --- a/src/coreclr/src/jit/emitxarch.cpp +++ b/src/coreclr/src/jit/emitxarch.cpp @@ -287,6 +287,7 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, bool needsOCF { return false; } + __fallthrough; // these always set OC to 0 case INS_and: case INS_or: From 67b6b59aa12eb1f02cd4edd379a056518f9e10ef Mon Sep 17 00:00:00 2001 From: Nathan Moore Date: Mon, 13 Jul 2020 19:25:57 -0400 Subject: [PATCH 7/7] Formatting --- src/coreclr/src/jit/emitxarch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/src/jit/emitxarch.cpp b/src/coreclr/src/jit/emitxarch.cpp index 1d23ae36eebd7b..90193e40475528 100644 --- a/src/coreclr/src/jit/emitxarch.cpp +++ b/src/coreclr/src/jit/emitxarch.cpp @@ -241,7 +241,7 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, bool needsOCF instrDesc* id = emitLastIns; insFormat fmt = id->idInsFmt(); - //make sure op1 is a reg + // make sure op1 is a reg switch (fmt) { case IF_RWR_CNS: