Made the changes suggested fromn code reviewers
authorBrian Sullivan <briansul@microsoft.com>
Fri, 19 Feb 2016 02:34:44 +0000 (18:34 -0800)
committerBrian Sullivan <briansul@microsoft.com>
Fri, 19 Feb 2016 02:34:44 +0000 (18:34 -0800)
Commit migrated from https://github.com/dotnet/coreclr/commit/a5b35b3b3ecef6370812a3c16b4f344c21217e71

src/coreclr/src/jit/codegenarm64.cpp
src/coreclr/src/jit/codegencommon.cpp
src/coreclr/src/jit/codegenlegacy.cpp
src/coreclr/src/jit/codegenxarch.cpp

index 16805a8..07acd9f 100644 (file)
@@ -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;
     }
index a2f262f..de2339e 100644 (file)
@@ -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;
index 8491292..57807fb 100644 (file)
@@ -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)
                     {
index 06b6d10..e329bdb 100644 (file)
@@ -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);
     }
 }