From a5b35b3b3ecef6370812a3c16b4f344c21217e71 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Thu, 18 Feb 2016 18:34:44 -0800 Subject: [PATCH] Made the changes suggested fromn code reviewers --- src/jit/codegenarm64.cpp | 63 ++++++++++++++++++++++++----------- src/jit/codegencommon.cpp | 84 +++++++++++++++++++++++------------------------ src/jit/codegenlegacy.cpp | 28 +++++++++------- src/jit/codegenxarch.cpp | 55 +++++++++++++++++++++++-------- 4 files changed, 143 insertions(+), 87 deletions(-) diff --git a/src/jit/codegenarm64.cpp b/src/jit/codegenarm64.cpp index 16805a8..07acd9f 100644 --- a/src/jit/codegenarm64.cpp +++ b/src/jit/codegenarm64.cpp @@ -2955,13 +2955,11 @@ CodeGen::genCodeForTreeNode(GenTreePtr treeNode) emitJumpKind jumpKind[2]; bool branchToTrueLabel[2]; genJumpKindsForTree(cmp, jumpKind, branchToTrueLabel); + assert(jumpKind[0] != EJ_NONE); - if (jumpKind[0] != EJ_NONE) - { - // On Arm64 the branches will always branch to the true label - assert(branchToTrueLabel[0]); - inst_JMP(jumpKind[0], compiler->compCurBB->bbJumpDest); - } + // On Arm64 the branches will always branch to the true label + assert(branchToTrueLabel[0]); + inst_JMP(jumpKind[0], compiler->compCurBB->bbJumpDest); if (jumpKind[1] != EJ_NONE) { @@ -5750,12 +5748,25 @@ void CodeGen::genLeaInstruction(GenTreeAddrMode *lea) genProduceReg(lea); } -/***************************************************************************** - * The condition to use for (the jmp/set for) the given type of compare operation are - * returned in 'jmpKind' array. - * If only one branch is necessary the value of jmpKind[1] will be EJ_NONE - * On Arm64 both branches will always branch to the true label - */ +//------------------------------------------------------------------------------------------- +// genJumpKindsForTree: Determine the number and kinds of conditional branches +// necessary to implement the given GT_CMP node +// +// Arguments: +// cmpTree - (input) The GenTree node that is used to set the Condition codes +// - The GenTree Relop node that was used to set the Condition codes +// jmpKind[2] - (output) One or two conditional branch instructions +// jmpToTrueLabel[2] - (output) On Arm64 both branches will always branch to the true label +// +// Return Value: +// Sets the proper values into the array elements of jmpKind[] and jmpToTrueLabel[] +// +// Assumptions: +// At least one conditional branch instruction will be returned. +// Typically only one conditional branch is needed +// and the second jmpKind[] value is set to EJ_NONE +//------------------------------------------------------------------------------------------- + // static void CodeGen::genJumpKindsForTree(GenTreePtr cmpTree, emitJumpKind jmpKind[2], @@ -5864,14 +5875,26 @@ void CodeGen::genJumpKindsForTree(GenTreePtr cmpTree, } } -// Generate code to materialize a condition into a register -// (the condition codes must already have been appropriately set) +//------------------------------------------------------------------------------------------- +// genSetRegToCond: Set a register 'dstReg' to the appropriate one or zero value +// corresponding to a binary Relational operator result. +// +// Arguments: +// dstReg - The target register to set to 1 or 0 +// tree - The GenTree Relop node that was used to set the Condition codes +// +// Return Value: none +// +// Notes: +// A full 64-bit value of either 1 or 0 is setup in the 'dstReg' +//------------------------------------------------------------------------------------------- void CodeGen::genSetRegToCond(regNumber dstReg, GenTreePtr tree) { emitJumpKind jumpKind[2]; bool branchToTrueLabel[2]; genJumpKindsForTree(tree, jumpKind, branchToTrueLabel); + assert(jumpKind[0] != EJ_NONE); // Set the reg according to the flags inst_SET(jumpKind[0], dstReg); @@ -5965,8 +5988,8 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode) { // We only need to check for a negative value in sourceReg emit->emitIns_R_I(INS_cmp, cmpSize, sourceReg, 0); - emitJumpKind jmpLTS = genJumpKindForOper(GT_LT, CK_SIGNED); - genJumpToThrowHlpBlk(jmpLTS, SCK_OVERFLOW); + emitJumpKind jmpLT = genJumpKindForOper(GT_LT, CK_SIGNED); + genJumpToThrowHlpBlk(jmpLT, SCK_OVERFLOW); if (dstType == TYP_ULONG) { // cast to TYP_ULONG: @@ -6007,8 +6030,8 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode) emit->emitIns_R_R(INS_cmp, cmpSize, sourceReg, tmpReg); } - emitJumpKind jmpGTS = genJumpKindForOper(GT_GT, CK_SIGNED); - genJumpToThrowHlpBlk(jmpGTS, SCK_OVERFLOW); + emitJumpKind jmpGT = genJumpKindForOper(GT_GT, CK_SIGNED); + genJumpToThrowHlpBlk(jmpGT, SCK_OVERFLOW); // Compare with the MIN @@ -6023,8 +6046,8 @@ void CodeGen::genIntToIntCast(GenTreePtr treeNode) emit->emitIns_R_R(INS_cmp, cmpSize, sourceReg, tmpReg); } - emitJumpKind jmpLTS = genJumpKindForOper(GT_LT, CK_SIGNED); - genJumpToThrowHlpBlk(jmpLTS, SCK_OVERFLOW); + emitJumpKind jmpLT = genJumpKindForOper(GT_LT, CK_SIGNED); + genJumpToThrowHlpBlk(jmpLT, SCK_OVERFLOW); } ins = INS_mov; } diff --git a/src/jit/codegencommon.cpp b/src/jit/codegencommon.cpp index a2f262f..de2339e 100644 --- a/src/jit/codegencommon.cpp +++ b/src/jit/codegencommon.cpp @@ -2457,7 +2457,7 @@ FOUND_AM: emitJumpKind CodeGen::genJumpKindForOper(genTreeOps cmp, CompareKind compareKind) { const static - BYTE genJCCinsSgn[] = + BYTE genJCCinsSigned[] = { #if defined(_TARGET_XARCH_) EJ_je, // GT_EQ @@ -2477,7 +2477,7 @@ emitJumpKind CodeGen::genJumpKindForOper(genTreeOps cmp, CompareKind co }; const static - BYTE genJCCinsUns[] = /* unsigned comparison */ + BYTE genJCCinsUnsigned[] = /* unsigned comparison */ { #if defined(_TARGET_XARCH_) EJ_je, // GT_EQ @@ -2497,7 +2497,7 @@ emitJumpKind CodeGen::genJumpKindForOper(genTreeOps cmp, CompareKind co }; const static - BYTE genJCCinsLog[] = /* logical operation */ + BYTE genJCCinsLogical[] = /* logical operation */ { #if defined(_TARGET_XARCH_) EJ_je, // GT_EQ (Z == 1) @@ -2517,43 +2517,43 @@ emitJumpKind CodeGen::genJumpKindForOper(genTreeOps cmp, CompareKind co }; #if defined(_TARGET_XARCH_) - assert(genJCCinsSgn[GT_EQ - GT_EQ] == EJ_je); - assert(genJCCinsSgn[GT_NE - GT_EQ] == EJ_jne); - assert(genJCCinsSgn[GT_LT - GT_EQ] == EJ_jl); - assert(genJCCinsSgn[GT_LE - GT_EQ] == EJ_jle); - assert(genJCCinsSgn[GT_GE - GT_EQ] == EJ_jge); - assert(genJCCinsSgn[GT_GT - GT_EQ] == EJ_jg); - - assert(genJCCinsUns[GT_EQ - GT_EQ] == EJ_je); - assert(genJCCinsUns[GT_NE - GT_EQ] == EJ_jne); - assert(genJCCinsUns[GT_LT - GT_EQ] == EJ_jb); - assert(genJCCinsUns[GT_LE - GT_EQ] == EJ_jbe); - assert(genJCCinsUns[GT_GE - GT_EQ] == EJ_jae); - assert(genJCCinsUns[GT_GT - GT_EQ] == EJ_ja); - - assert(genJCCinsLog[GT_EQ - GT_EQ] == EJ_je); - assert(genJCCinsLog[GT_NE - GT_EQ] == EJ_jne); - assert(genJCCinsLog[GT_LT - GT_EQ] == EJ_js); - assert(genJCCinsLog[GT_GE - GT_EQ] == EJ_jns); + assert(genJCCinsSigned[GT_EQ - GT_EQ] == EJ_je); + assert(genJCCinsSigned[GT_NE - GT_EQ] == EJ_jne); + assert(genJCCinsSigned[GT_LT - GT_EQ] == EJ_jl); + assert(genJCCinsSigned[GT_LE - GT_EQ] == EJ_jle); + assert(genJCCinsSigned[GT_GE - GT_EQ] == EJ_jge); + assert(genJCCinsSigned[GT_GT - GT_EQ] == EJ_jg); + + assert(genJCCinsUnsigned[GT_EQ - GT_EQ] == EJ_je); + assert(genJCCinsUnsigned[GT_NE - GT_EQ] == EJ_jne); + assert(genJCCinsUnsigned[GT_LT - GT_EQ] == EJ_jb); + assert(genJCCinsUnsigned[GT_LE - GT_EQ] == EJ_jbe); + assert(genJCCinsUnsigned[GT_GE - GT_EQ] == EJ_jae); + assert(genJCCinsUnsigned[GT_GT - GT_EQ] == EJ_ja); + + assert(genJCCinsLogical[GT_EQ - GT_EQ] == EJ_je); + assert(genJCCinsLogical[GT_NE - GT_EQ] == EJ_jne); + assert(genJCCinsLogical[GT_LT - GT_EQ] == EJ_js); + assert(genJCCinsLogical[GT_GE - GT_EQ] == EJ_jns); #elif defined(_TARGET_ARMARCH_) - assert(genJCCinsSgn[GT_EQ - GT_EQ] == EJ_eq); - assert(genJCCinsSgn[GT_NE - GT_EQ] == EJ_ne); - assert(genJCCinsSgn[GT_LT - GT_EQ] == EJ_lt); - assert(genJCCinsSgn[GT_LE - GT_EQ] == EJ_le); - assert(genJCCinsSgn[GT_GE - GT_EQ] == EJ_ge); - assert(genJCCinsSgn[GT_GT - GT_EQ] == EJ_gt); - - assert(genJCCinsUns[GT_EQ - GT_EQ] == EJ_eq); - assert(genJCCinsUns[GT_NE - GT_EQ] == EJ_ne); - assert(genJCCinsUns[GT_LT - GT_EQ] == EJ_lo); - assert(genJCCinsUns[GT_LE - GT_EQ] == EJ_ls); - assert(genJCCinsUns[GT_GE - GT_EQ] == EJ_hs); - assert(genJCCinsUns[GT_GT - GT_EQ] == EJ_hi); - - assert(genJCCinsLog[GT_EQ - GT_EQ] == EJ_eq); - assert(genJCCinsLog[GT_NE - GT_EQ] == EJ_ne); - assert(genJCCinsLog[GT_LT - GT_EQ] == EJ_mi); - assert(genJCCinsLog[GT_GE - GT_EQ] == EJ_pl); + assert(genJCCinsSigned[GT_EQ - GT_EQ] == EJ_eq); + assert(genJCCinsSigned[GT_NE - GT_EQ] == EJ_ne); + assert(genJCCinsSigned[GT_LT - GT_EQ] == EJ_lt); + assert(genJCCinsSigned[GT_LE - GT_EQ] == EJ_le); + assert(genJCCinsSigned[GT_GE - GT_EQ] == EJ_ge); + assert(genJCCinsSigned[GT_GT - GT_EQ] == EJ_gt); + + assert(genJCCinsUnsigned[GT_EQ - GT_EQ] == EJ_eq); + assert(genJCCinsUnsigned[GT_NE - GT_EQ] == EJ_ne); + assert(genJCCinsUnsigned[GT_LT - GT_EQ] == EJ_lo); + assert(genJCCinsUnsigned[GT_LE - GT_EQ] == EJ_ls); + assert(genJCCinsUnsigned[GT_GE - GT_EQ] == EJ_hs); + assert(genJCCinsUnsigned[GT_GT - GT_EQ] == EJ_hi); + + assert(genJCCinsLogical[GT_EQ - GT_EQ] == EJ_eq); + assert(genJCCinsLogical[GT_NE - GT_EQ] == EJ_ne); + assert(genJCCinsLogical[GT_LT - GT_EQ] == EJ_mi); + assert(genJCCinsLogical[GT_GE - GT_EQ] == EJ_pl); #else assert(!"unknown arch"); #endif @@ -2563,15 +2563,15 @@ emitJumpKind CodeGen::genJumpKindForOper(genTreeOps cmp, CompareKind co if (compareKind == CK_UNSIGNED) { - result = (emitJumpKind)genJCCinsUns[cmp - GT_EQ]; + result = (emitJumpKind)genJCCinsUnsigned[cmp - GT_EQ]; } else if (compareKind == CK_SIGNED) { - result = (emitJumpKind)genJCCinsSgn[cmp - GT_EQ]; + result = (emitJumpKind)genJCCinsSigned[cmp - GT_EQ]; } else if (compareKind == CK_LOGICAL) { - result = (emitJumpKind)genJCCinsLog[cmp - GT_EQ]; + result = (emitJumpKind)genJCCinsLogical[cmp - GT_EQ]; } assert(result != EJ_COUNT); return result; diff --git a/src/jit/codegenlegacy.cpp b/src/jit/codegenlegacy.cpp index 8491292..57807fb 100644 --- a/src/jit/codegenlegacy.cpp +++ b/src/jit/codegenlegacy.cpp @@ -3487,10 +3487,12 @@ regMaskTP CodeGen::WriteBarrier(GenTreePtr tgt, void CodeGen::genJccLongHi(genTreeOps cmp, BasicBlock * jumpTrue, BasicBlock * jumpFalse, - bool unsOper ) + bool isUnsigned ) { if (cmp != GT_NE) + { jumpFalse->bbFlags |= BBF_JMP_TARGET|BBF_HAS_LABEL; + } switch (cmp) { @@ -3504,7 +3506,7 @@ void CodeGen::genJccLongHi(genTreeOps cmp, case GT_LT: case GT_LE: - if (unsOper) + if (isUnsigned) { inst_JMP(EJ_ja , jumpFalse); inst_JMP(EJ_jb , jumpTrue); @@ -3518,7 +3520,7 @@ void CodeGen::genJccLongHi(genTreeOps cmp, case GT_GE: case GT_GT: - if (unsOper) + if (isUnsigned) { inst_JMP(EJ_jb , jumpFalse); inst_JMP(EJ_ja , jumpTrue); @@ -3583,12 +3585,14 @@ void CodeGen::genJccLongLo(genTreeOps cmp, */ void CodeGen::genJccLongHi(genTreeOps cmp, - BasicBlock * jumpTrue, - BasicBlock * jumpFalse, - bool unsOper) + BasicBlock * jumpTrue, + BasicBlock * jumpFalse, + bool isUnsigned) { if (cmp != GT_NE) + { jumpFalse->bbFlags |= BBF_JMP_TARGET | BBF_HAS_LABEL; + } switch (cmp) { @@ -3602,7 +3606,7 @@ void CodeGen::genJccLongHi(genTreeOps cmp, case GT_LT: case GT_LE: - if (unsOper) + if (isUnsigned) { inst_JMP(EJ_hi, jumpFalse); inst_JMP(EJ_lo, jumpTrue); @@ -3616,7 +3620,7 @@ void CodeGen::genJccLongHi(genTreeOps cmp, case GT_GE: case GT_GT: - if (unsOper) + if (isUnsigned) { inst_JMP(EJ_lo, jumpFalse); inst_JMP(EJ_hi, jumpTrue); @@ -3640,8 +3644,8 @@ void CodeGen::genJccLongHi(genTreeOps cmp, */ void CodeGen::genJccLongLo(genTreeOps cmp, - BasicBlock* jumpTrue, - BasicBlock* jumpFalse) + BasicBlock* jumpTrue, + BasicBlock* jumpFalse) { switch (cmp) { @@ -15326,8 +15330,8 @@ USE_SAR_FOR_CAST: genComputeRegPair(op1, REG_PAIR_NONE, RBM_ALLINT & ~needReg, RegSet::FREE_REG); regPair = op1->gtRegPair; - // Do we need to set the sign-flag, or can be check if it - // set, and not do this "test" if so. + // Do we need to set the sign-flag, or can we checked if it is set? + // and not do this "test" if so. if (op1->gtFlags & GTF_REG_VAL) { diff --git a/src/jit/codegenxarch.cpp b/src/jit/codegenxarch.cpp index 06b6d10..e329bdb 100644 --- a/src/jit/codegenxarch.cpp +++ b/src/jit/codegenxarch.cpp @@ -6261,14 +6261,31 @@ void CodeGen::genLeaInstruction(GenTreeAddrMode *lea) genProduceReg(lea); } -/***************************************************************************** - * The condition to use for (the jmp/set for) the given type of compare operation are - * returned in 'jmpKind' array. The corresponding elements of jmpToTrueLabel indicate - * the branch target when the condition being true. - * - * jmpToTrueLabel[i]= true implies branch to the target when the compare operation is true. - * jmpToTrueLabel[i]= false implies branch to the target when the compare operation is false. - */ +//------------------------------------------------------------------------------------------- +// genJumpKindsForTree: Determine the number and kinds of conditional branches +// necessary to implement the given GT_CMP node +// +// Arguments: +// cmpTree - (input) The GenTree node that is used to set the Condition codes +// - The GenTree Relop node that was used to set the Condition codes +// jmpKind[2] - (output) One or two conditional branch instructions +// jmpToTrueLabel[2] - (output) When true we branch to the true case +// When false we create a second label and branch to the false case +// Only GT_EQ for a floating point compares can have a false value. +// +// Return Value: +// Sets the proper values into the array elements of jmpKind[] and jmpToTrueLabel[] +// +// Assumptions: +// At least one conditional branch instruction will be returned. +// Typically only one conditional branch is needed +// and the second jmpKind[] value is set to EJ_NONE +// +// Notes: +// jmpToTrueLabel[i]= true implies branch when the compare operation is true. +// jmpToTrueLabel[i]= false implies branch when the compare operation is false. +//------------------------------------------------------------------------------------------- + // static void CodeGen::genJumpKindsForTree(GenTreePtr cmpTree, emitJumpKind jmpKind[2], @@ -6293,7 +6310,7 @@ void CodeGen::genJumpKindsForTree(GenTreePtr cmpTree, // while generating code for compare opererators (e.g. GT_EQ etc). if ((cmpTree->gtFlags & GTF_RELOP_NAN_UN) != 0) { - // Unordered + // Must branch if we have an NaN, unordered switch (cmpTree->gtOper) { case GT_LT: @@ -6322,8 +6339,9 @@ void CodeGen::genJumpKindsForTree(GenTreePtr cmpTree, unreached(); } } - else + else // ((cmpTree->gtFlags & GTF_RELOP_NAN_UN) == 0) { + // Do not branch if we have an NaN, unordered switch (cmpTree->gtOper) { case GT_LT: @@ -6825,8 +6843,20 @@ void CodeGen::genCompareInt(GenTreePtr treeNode) genProduceReg(tree); } } -// Generate code to materialize a condition into a register -// (the condition codes must already have been appropriately set) + +//------------------------------------------------------------------------------------------- +// genSetRegToCond: Set a register 'dstReg' to the appropriate one or zero value +// corresponding to a binary Relational operator result. +// +// Arguments: +// dstReg - The target register to set to 1 or 0 +// tree - The GenTree Relop node that was used to set the Condition codes +// +// Return Value: none +// +// Notes: +// A full 64-bit value of either 1 or 0 is setup in the 'dstReg' +//------------------------------------------------------------------------------------------- void CodeGen::genSetRegToCond(regNumber dstReg, GenTreePtr tree) { @@ -6894,7 +6924,6 @@ void CodeGen::genSetRegToCond(regNumber dstReg, GenTreePtr tree) } else { - // FP types have been converted to flow noway_assert(treeType == TYP_BYTE); } } -- 2.7.4