Followup work from codereview feedback

For PR  #6467 - Enable multireg returns on Arm64

8 files changed, 90 insertions, 103 deletions

diff --git a/src/jit/codegenarm64.cpp b/src/jit/codegenarm64.cpp
index 3e505bbff0..188f646e05 100644
--- a/src/jit/codegenarm64.cpp
+++ b/src/jit/codegenarm64.cpp
@@ -5441,7 +5441,7 @@ void CodeGen::genCallInstruction(GenTreePtr node)
                     inst_RV_RV(ins_Move_Extend(putArgRegNode->TypeGet(), putArgRegNode->InReg()), argReg, putArgRegNode->gtRegNum);
                 }
 
-                argReg = REG_NEXT(argReg);
+                argReg = genRegArgNext(argReg);
             }
         }
         else
@@ -5855,13 +5855,11 @@ void CodeGen::genJmpMethod(GenTreePtr jmp)
                 if (varDsc->lvIsHfa())
                 {
                     NYI_ARM64("CodeGen::genJmpMethod with multireg HFA arg");
-                    // use genRegArgNextFloat
-                }
-                else
-                {
-                    // Restore the second register.
-                    argRegNext = genRegArgNext(argReg);
                 }
+
+                // Restore the second register.
+                argRegNext = genRegArgNext(argReg);
+
                 loadType = compiler->getJitGCType(varDsc->lvGcLayout[1]);
                 loadSize = emitActualTypeSize(loadType);
                 getEmitter()->emitIns_R_S(ins_Load(loadType), loadSize, argRegNext, varNum, TARGET_POINTER_SIZE);
diff --git a/src/jit/codegenlegacy.cpp b/src/jit/codegenlegacy.cpp
index 476f8af790..a43568038a 100644
--- a/src/jit/codegenlegacy.cpp
+++ b/src/jit/codegenlegacy.cpp
@@ -18078,14 +18078,7 @@ void CodeGen::SetupLateArgs(GenTreePtr call)
                     assert(genRegMask(regNum2) & RBM_CALLEE_TRASH);
                     regSet.rsSpillReg(regNum2);
                 }
-                if (isValidIntArgReg(regNum2))
-                {
-                    regNum2 = genRegArgNext(regNum2);
-                }
-                else
-                {
-                    regNum2 = genRegArgNextFloat(regNum2);
-                }
+                regNum2 = genRegArgNext(regNum2);
                 assert(i + 1 == curArgTabEntry->numRegs || regNum2 != MAX_REG_ARG);
             }
 
diff --git a/src/jit/lowerarm64.cpp b/src/jit/lowerarm64.cpp
index 340bdf8561..9915872654 100644
--- a/src/jit/lowerarm64.cpp
+++ b/src/jit/lowerarm64.cpp
@@ -1032,7 +1032,7 @@ Lowering::TreeNodeInfoInitCall(GenTreeCall* call)
                     iterationNum++;
 
                     // Update targetReg and targetMask for the next putarg_reg (if any)
-                    targetReg  = REG_NEXT(targetReg);
+                    targetReg  = genRegArgNext(targetReg);
                     targetMask = genRegMask(targetReg);
                 }
             }
@@ -1057,7 +1057,7 @@ Lowering::TreeNodeInfoInitCall(GenTreeCall* call)
                 if (curReg == lastReg)
                     break;
 
-                curReg = REG_NEXT(curReg);
+                curReg = genRegArgNext(curReg);
             }
 
             // Struct typed arguments must be fully passed in registers (Reg/Stk split not allowed)
diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp
index 902fce37cc..1754f6f9fa 100755
--- a/src/jit/morph.cpp
+++ b/src/jit/morph.cpp
@@ -2604,12 +2604,10 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* callNode)
     unsigned        intArgRegNum  = 0;
     unsigned        fltArgRegNum  = 0;
 
+#ifdef _TARGET_ARM_
     regMaskTP       argSkippedRegMask    = RBM_NONE;
-
-#if defined(_TARGET_ARM_) || defined(_TARGET_AMD64_)
-    // Note this in ONLY used by _TARGET_ARM_
     regMaskTP       fltArgSkippedRegMask = RBM_NONE;
-#endif
+#endif //  _TARGET_ARM_
 
 #if defined(_TARGET_X86_)
     unsigned        maxRegArgs    = MAX_REG_ARG;  // X86: non-const, must be calculated
@@ -2953,23 +2951,19 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* callNode)
         // this can't be a struct.
         assert(argx->gtType != TYP_STRUCT);
 
-        // FIXME: Issue #4025 Why do we need floating type for 'this' argument
         /* Increment the argument register count and argument index */
         if (!varTypeIsFloating(argx->gtType) || opts.compUseSoftFP)
         {
             intArgRegNum++;
-#if defined(_TARGET_AMD64_) && !defined(UNIX_AMD64_ABI)
-            fltArgSkippedRegMask |= genMapArgNumToRegMask(fltArgRegNum, TYP_FLOAT);
+#ifdef WINDOWS_AMD64_ABI
+            // Whenever we pass an integer register argument
+            // we skip the corresponding floating point register argument  
             fltArgRegNum++;
-#endif
+#endif // WINDOWS_AMD64_ABI
         }
         else
         {
-            fltArgRegNum++;
-#if defined(_TARGET_AMD64_) && !defined(UNIX_AMD64_ABI)
-            argSkippedRegMask |= genMapArgNumToRegMask(intArgRegNum, TYP_I_IMPL);
-            intArgRegNum++;
-#endif
+            noway_assert(!"the 'this' pointer can not be a floating point type");
         }
         argIndex++;
         argSlots++;
@@ -3203,7 +3197,7 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* callNode)
         {
             passUsingFloatRegs = varTypeIsFloating(argx);
         }
-#else // !UNIX_AMD64_ABI
+#else // WINDOWS_AMD64_ABI
         passUsingFloatRegs = varTypeIsFloating(argx);
 #endif // !UNIX_AMD64_ABI
 #elif defined(_TARGET_X86_)
@@ -3980,10 +3974,11 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* callNode)
                     {
                         fltArgRegNum += size;
 
-#if defined(_TARGET_AMD64_) && !defined(UNIX_AMD64_ABI)
-                        argSkippedRegMask |= genMapArgNumToRegMask(intArgRegNum, TYP_I_IMPL);
+#ifdef WINDOWS_AMD64_ABI
+                        // Whenever we pass an integer register argument
+                        // we skip the corresponding floating point register argument  
                         intArgRegNum = min(intArgRegNum + size, MAX_REG_ARG);
-#endif // _TARGET_AMD64_
+#endif // WINDOWS_AMD64_ABI
 #ifdef _TARGET_ARM_
                         if (fltArgRegNum > MAX_FLOAT_REG_ARG)
                         {
@@ -4011,7 +4006,6 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* callNode)
                         }
 
 #if defined(_TARGET_AMD64_) && !defined(UNIX_AMD64_ABI)
-                        fltArgSkippedRegMask |= genMapArgNumToRegMask(fltArgRegNum, TYP_DOUBLE);
                         fltArgRegNum = min(fltArgRegNum + size, MAX_FLOAT_REG_ARG);
 #endif // _TARGET_AMD64_
 #ifdef _TARGET_ARM_
@@ -4118,7 +4112,10 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* callNode)
     {
         call->fgArgInfo->ArgsComplete();
 #ifdef LEGACY_BACKEND
-        call->gtCallRegUsedMask = genIntAllRegArgMask(intArgRegNum) & ~argSkippedRegMask;
+        call->gtCallRegUsedMask = genIntAllRegArgMask(intArgRegNum);
+#if defined(_TARGET_ARM_)
+        call->gtCallRegUsedMask &= ~argSkippedRegMask;
+#endif
         if (fltArgRegNum > 0)
         {
 #if defined(_TARGET_ARM_)
@@ -4492,7 +4489,7 @@ void Compiler::fgMorphMultiregStructArgs(GenTreeCall* call)
 #ifdef _TARGET_AMD64_
 #if defined(UNIX_AMD64_ABI)
     NYI_AMD64("fgMorphMultiregStructArgs (UNIX ABI)");
-#else // !UNIX_AMD64_ABI
+#else // WINDOWS_AMD64_ABI
     assert(!"Logic error: no MultiregStructArgs for Windows X64 ABI");
 #endif // !UNIX_AMD64_ABI
 #endif
diff --git a/src/jit/optcse.cpp b/src/jit/optcse.cpp
index b93930b8ae..424939d9a2 100644
--- a/src/jit/optcse.cpp
+++ b/src/jit/optcse.cpp
@@ -381,7 +381,8 @@ bool                Compiler::optCSE_canSwap(GenTreePtr tree)
 
 /*****************************************************************************
  *
- *  Compare function passed to qsort() by optLexicalOptimizeCSEs().
+ *  Compare function passed to qsort() by CSE_Heuristic::SortCandidates
+ *  when (CodeOptKind() != Compiler::SMALL_CODE)
  */
 
 /* static */
@@ -412,13 +413,14 @@ int __cdecl         Compiler::optCSEcostCmpEx(const void *op1, const void *op2)
     if (diff != 0)
         return diff;
 
-    // In order to ensure that we have a stable sort the lower csdIndex towards to the top
+    // In order to ensure that we have a stable sort, we break ties using the csdIndex
     return (int) (dsc1->csdIndex - dsc2->csdIndex);
 }
 
 /*****************************************************************************
  *
- *  Compare function passed to qsort() by optLexicalOptimizeCSEs().
+ *  Compare function passed to qsort() by CSE_Heuristic::SortCandidates
+ *  when (CodeOptKind() == Compiler::SMALL_CODE)
  */
 
 /* static */
@@ -449,7 +451,7 @@ int __cdecl         Compiler::optCSEcostCmpSz(const void *op1, const void *op2)
     if (diff != 0)
         return diff;
 
-    // In order to ensure that we have a stable sort the lower csdIndex towards to the top
+    // In order to ensure that we have a stable sort, we break ties using the csdIndex
     return (int)(dsc1->csdIndex - dsc2->csdIndex);
 }
 
diff --git a/src/jit/registerfp.cpp b/src/jit/registerfp.cpp
index 9b7f651d41..1774b8a9e7 100644
--- a/src/jit/registerfp.cpp
+++ b/src/jit/registerfp.cpp
@@ -25,7 +25,7 @@ regNumber alignFloatArgReg(regNumber argReg, int alignment)
 
     int regsize_alignment = alignment /= REGSIZE_BYTES;
     if (genMapFloatRegNumToRegArgNum(argReg) % regsize_alignment)
-        argReg = genRegArgNextFloat(argReg);
+        argReg = genRegArgNext(argReg);
     
     // technically the above should be a 'while' so make sure 
     // we never should have incremented more than once
diff --git a/src/jit/regset.cpp b/src/jit/regset.cpp
index 9d31913450..b74b101896 100644
--- a/src/jit/regset.cpp
+++ b/src/jit/regset.cpp
@@ -1231,11 +1231,11 @@ void                RegSet::rsMultRegFree(regMaskTP regMask)
 #ifdef _TARGET_ARM_
                 if (genIsValidFloatReg(regNum) && (type == TYP_DOUBLE))
                 {
-                    // Skip the second register for a TYP_DOUBLE
+                    // On ARM32, We skip the second register for a TYP_DOUBLE
                     regNum = REG_NEXT(regNum);
                     regBit <<= 1;
                 }
-#endif
+#endif // _TARGET_ARM_
             }
         }
     }
@@ -3467,63 +3467,54 @@ bool                genIsProperRegPair(regPairNo regPair)
 
 regNumber           genRegArgNext(regNumber argReg)
 {
-    assert(isValidIntArgReg(argReg));
-
     regNumber result = REG_NA;
 
-#ifdef _TARGET_AMD64_
-#ifdef UNIX_AMD64_ABI
-    // Windows X64 ABI:
-    //     REG_EDI, REG_ESI, REG_ECX, REG_EDX, REG_R8, REG_R9
-    //
-    if (argReg == REG_ARG_1)       // REG_ESI
+    if (isValidFloatArgReg(argReg))
     {
-        result = REG_ARG_2;        // REG_ECX
+        // We can iterate the floating point argument registers by using +1
+        result = REG_NEXT(argReg);
     }
-    else if (argReg == REG_ARG_3)  // REG_EDX
+    else
     {
-        result = REG_ARG_4;        // REG_R8
-    }
+        assert(isValidIntArgReg(argReg));
+
+#ifdef _TARGET_AMD64_
+#ifdef UNIX_AMD64_ABI
+        // Windows X64 ABI:
+        //     REG_EDI, REG_ESI, REG_ECX, REG_EDX, REG_R8, REG_R9
+        //
+        if (argReg == REG_ARG_1)       // REG_ESI
+        {
+            result = REG_ARG_2;        // REG_ECX
+        }
+        else if (argReg == REG_ARG_3)  // REG_EDX
+        {
+            result = REG_ARG_4;        // REG_R8
+        }
 #else // Windows ABI
-    // Windows X64 ABI:
-    //     REG_ECX, REG_EDX, REG_R8, REG_R9
-    //
-    if (argReg == REG_ARG_1)       // REG_EDX
-    {
-        result = REG_ARG_2;        // REG_R8
-    }
+        // Windows X64 ABI:
+        //     REG_ECX, REG_EDX, REG_R8, REG_R9
+        //
+        if (argReg == REG_ARG_1)       // REG_EDX
+        {
+            result = REG_ARG_2;        // REG_R8
+        }
 #endif // UNIX or Windows ABI
 #endif // _TARGET_AMD64_
-
-    // If we didn't set 'result' to valid register above 
-    // then we will just iterate 'argReg' using REG_NEXT 
-    //
-    if (result == REG_NA)
-    {
-        // Otherwise we just iterate the argument registers by using REG_NEXT
-        result = REG_NEXT(argReg);
+        
+        // If we didn't set 'result' to valid register above 
+        // then we will just iterate 'argReg' using REG_NEXT 
+        //
+        if (result == REG_NA)
+        {
+            // Otherwise we just iterate the argument registers by using REG_NEXT
+            result = REG_NEXT(argReg);
+        }
     }
 
     return result;
 }
 
-#if !defined(_TARGET_X86_)
-
-/*****************************************************************************
- *
- *  Returns true if the register is a valid argument register 
- */
-
-regNumber           genRegArgNextFloat(regNumber argReg)
-{
-    assert(isValidFloatArgReg(argReg));
-
-    // We can iterate the floating point argument registers by using +1
-    return REG_NEXT(argReg);
-}
-
-#endif // !defined(_TARGET_X86_)
-
 /*****************************************************************************
  *
  *  The following table determines the order in which callee-saved registers
diff --git a/src/jit/target.h b/src/jit/target.h
index 71de326dc2..0d1a6f26c6 100644
--- a/src/jit/target.h
+++ b/src/jit/target.h
@@ -476,6 +476,9 @@ typedef unsigned short          regPairNoSmall; // arm: need 12 bits
   #define RBM_ALLFLOAT            (RBM_FPV0 | RBM_FPV1 | RBM_FPV2 | RBM_FPV3 | RBM_FPV4 | RBM_FPV5 | RBM_FPV6)
   #define REG_FP_FIRST             REG_FPV0
   #define REG_FP_LAST              REG_FPV7
+  #define FIRST_FP_ARGREG          REG_NA
+  #define LAST_FP_ARGREG           REG_NA
+
 
   #define REGNUM_BITS              3       // number of bits in a REG_*
   #define TINY_REGNUM_BITS         3       
@@ -739,6 +742,7 @@ typedef unsigned short          regPairNoSmall; // arm: need 12 bits
   #define MAX_ARG_REG_COUNT             2  // Maximum registers used to pass a single argument in multiple registers.
   #define MAX_RET_REG_COUNT             2  // Maximum registers used to return a value.
 #else // !UNIX_AMD64_ABI
+  #define WINDOWS_AMD64_ABI                // Uses the Windows ABI for AMD64
   #define FEATURE_MULTIREG_ARGS_OR_RET  0  // Support for passing and/or returning single values in more than one register
   #define FEATURE_MULTIREG_ARGS         0  // Support for passing a single argument in more than one register  
   #define FEATURE_MULTIREG_RET          0  // Support for returning a single value in more than one register
@@ -2009,32 +2013,27 @@ inline bool         isValidIntArgReg(regNumber reg)
 
 //-------------------------------------------------------------------------------------------
 // genRegArgNext:
-//     Given a register that is an integer argument register 
-//     returns the next integer argument register 
+//     Given a register that is an integer or floating point argument register 
+//     returns the next argument register 
 //
 regNumber           genRegArgNext(regNumber argReg);
 
-#if !defined(_TARGET_X86_)
-
 //-------------------------------------------------------------------------------------------
 // isValidFloatArgReg:
 //     Returns true if the register is a valid floating-point argument register 
 //
 inline bool         isValidFloatArgReg(regNumber reg)
 {
-    return (reg >= FIRST_FP_ARGREG) && (reg <= LAST_FP_ARGREG);
+    if (reg == REG_NA)
+    {
+        return false;
+    }
+    else
+    {
+        return (reg >= FIRST_FP_ARGREG) && (reg <= LAST_FP_ARGREG);
+    }
 }
 
-//-------------------------------------------------------------------------------------------
-// genRegArgNextFloat:
-//     Given a register that is a floating-point argument register 
-//     returns the next floating-point argument register 
-//
-regNumber           genRegArgNextFloat(regNumber argReg);
-
-#endif // !defined(_TARGET_X86_)
-
-
 /*****************************************************************************
  *
  *  Can the register hold the argument type?
@@ -2306,6 +2305,13 @@ bool                isFloatRegType(int /* s/b "var_types" */ type)
 #endif
 }
 
+// If the WINDOWS_AMD64_ABI is defined make sure that _TARGET_AMD64_ is also defined.
+#if defined(WINDOWS_AMD64_ABI)
+#if !defined(_TARGET_AMD64_)
+#error When WINDOWS_AMD64_ABI is defined you must define _TARGET_AMD64_ defined as well.
+#endif
+#endif
+
 /*****************************************************************************/
 // Some sanity checks on some of the register masks
 // Stack pointer is never part of RBM_ALLINT