Enable genFnCalleeRegArgs for Arm64 Varargs (dotnet/coreclr#18714)

* Enable genFnCalleeRegArgs for Arm64 Varargs

Before the method would early out and incorrectly expect the usage
of all incoming arguments to be their homed stack slots. It is
instead possible for incoming arguments to be homed to different
integer registers.

The change will mangle the float types for vararg cases in the same
way that is done during lvaInitUserArgs and fgMorphArgs.

* Apply format patch

* Account for softfp case

* Address feedback

* Apply format patch

* Use standard function header for mangleVarArgsType

* Remove confusing comment

Commit migrated from https://github.com/dotnet/coreclr/commit/b1bda1bbc9a5fe9954938b4b627660d3acbe7504

diff --git a/src/coreclr/src/jit/codegencommon.cpp b/src/coreclr/src/jit/codegencommon.cpp
index 5ed6a63..bbe1dc7 100644 (file)
--- a/src/coreclr/src/jit/codegencommon.cpp
+++ b/src/coreclr/src/jit/codegencommon.cpp
@@ -3298,15 +3298,6 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere
     }
 #endif
 
-#if defined(_TARGET_WINDOWS_) && defined(_TARGET_ARM64_)
-    if (compiler->info.compIsVarArgs)
-    {
-        // We've already saved all int registers at the top of stack in the prolog.
-        // No need further action.
-        return;
-    }
-#endif // defined(_TARGET_WINDOWS_) && defined(_TARGET_ARM64_)
-
     unsigned  argMax;           // maximum argNum value plus 1, (including the RetBuffArg)
     unsigned  argNum;           // current argNum, always in [0..argMax-1]
     unsigned  fixedRetBufIndex; // argNum value used by the fixed return buffer argument (ARM64)
@@ -3401,13 +3392,18 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere
         // In other cases, we simply use the type of the lclVar to determine the type of the register.
         var_types getRegType(Compiler* compiler)
         {
-            LclVarDsc varDsc = compiler->lvaTable[varNum];
+            const LclVarDsc& varDsc = compiler->lvaTable[varNum];
             // Check if this is an HFA register arg and return the HFA type
             if (varDsc.lvIsHfaRegArg())
             {
+#if defined(_TARGET_WINDOWS_)
+                // Cannot have hfa types on windows arm targets
+                // in vararg methods.
+                assert(!compiler->info.compIsVarArgs);
+#endif // defined(_TARGET_WINDOWS_)
                 return varDsc.GetHfaType();
             }
-            return varDsc.lvType;
+            return compiler->mangleVarArgsType(varDsc.lvType);
         }
 
 #endif // !UNIX_AMD64_ABI
@@ -3415,8 +3411,11 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere
 
     unsigned   varNum;
     LclVarDsc* varDsc;
-    for (varNum = 0, varDsc = compiler->lvaTable; varNum < compiler->lvaCount; varNum++, varDsc++)
+
+    for (varNum = 0; varNum < compiler->lvaCount; ++varNum)
     {
+        varDsc = compiler->lvaTable + varNum;
+
         // Is this variable a register arg?
         if (!varDsc->lvIsParam)
         {
@@ -3466,7 +3465,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere
             }
         }
 
-        var_types regType = varDsc->TypeGet();
+        var_types regType = compiler->mangleVarArgsType(varDsc->TypeGet());
         // Change regType to the HFA type when we have a HFA argument
         if (varDsc->lvIsHfaRegArg())
         {
@@ -3661,11 +3660,6 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere
             regNumber regNum = genMapRegArgNumToRegNum(regArgNum + i, regType);
 
 #if !defined(UNIX_AMD64_ABI)
-            // lvArgReg could be INT or FLOAT reg. So the following assertion doesn't hold.
-            // The type of the register depends on the classification of the first eightbyte
-            // of the struct. For information on classification refer to the System V x86_64 ABI at:
-            // http://www.x86-64.org/documentation/abi.pdf
-
             assert((i > 0) || (regNum == varDsc->lvArgReg));
 #endif // defined(UNIX_AMD64_ABI)
             // Is the arg dead on entry to the method ?
@@ -3685,7 +3679,8 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere
                     // refcnt.  This is in contrast with the non-LSRA case in which all
                     // non-tracked args are assumed live on entry.
                     noway_assert((varDsc->lvRefCnt == 0) || (varDsc->lvType == TYP_STRUCT) ||
-                                 (varDsc->lvAddrExposed && compiler->info.compIsVarArgs));
+                                 (varDsc->lvAddrExposed && compiler->info.compIsVarArgs) ||
+                                 (varDsc->lvAddrExposed && compiler->opts.compUseSoftFP));
 #endif // !_TARGET_X86_
                 }
                 // Mark it as processed and be done with it
@@ -3971,7 +3966,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere
         }
         else // Not a struct type
         {
-            storeType = genActualType(varDsc->TypeGet());
+            storeType = compiler->mangleVarArgsType(genActualType(varDsc->TypeGet()));
         }
         size = emitActualTypeSize(storeType);
 #ifdef _TARGET_X86_

diff --git a/src/coreclr/src/jit/compiler.hpp b/src/coreclr/src/jit/compiler.hpp
index 7ae6460..270a593 100644 (file)
--- a/src/coreclr/src/jit/compiler.hpp
+++ b/src/coreclr/src/jit/compiler.hpp
@@ -2599,11 +2599,25 @@ inline unsigned Compiler::compMapILargNum(unsigned ILargNum)
     return (ILargNum);
 }
 
-// For ARM varargs, all arguments go in integer registers, so swizzle the type
+//------------------------------------------------------------------------
+// Compiler::mangleVarArgsType: Retype float types to their corresponding
+//                            : int/long types.
+//
+// Notes:
+//
+// The mangling of types will only occur for incoming vararg fixed arguments
+// on windows arm|64 or on armel (softFP).
+//
+// NO-OP for all other cases.
+//
 inline var_types Compiler::mangleVarArgsType(var_types type)
 {
-#ifdef _TARGET_ARMARCH_
-    if (info.compIsVarArgs || opts.compUseSoftFP)
+#if defined(_TARGET_ARMARCH_)
+    if (opts.compUseSoftFP
+#if defined(_TARGET_WINDOWS_)
+        || info.compIsVarArgs
+#endif // defined(_TARGET_WINDOWS_)
+        )
     {
         switch (type)
         {
@@ -2615,7 +2629,7 @@ inline var_types Compiler::mangleVarArgsType(var_types type)
                 break;
         }
     }
-#endif // _TARGET_ARMARCH_
+#endif // defined(_TARGET_ARMARCH_)
     return type;
 }
 

diff --git a/src/coreclr/src/jit/lclvars.cpp b/src/coreclr/src/jit/lclvars.cpp
index a6c6330..b9b5038 100644 (file)
--- a/src/coreclr/src/jit/lclvars.cpp
+++ b/src/coreclr/src/jit/lclvars.cpp
@@ -5912,13 +5912,14 @@ void Compiler::lvaAssignVirtualFrameOffsetsToLocals()
                 }
 
 #ifdef _TARGET_ARM64_
-                if (info.compIsVarArgs)
+                if (info.compIsVarArgs && varDsc->lvArgReg != theFixedRetBuffArgNum())
                 {
                     // Stack offset to varargs (parameters) should point to home area which will be preallocated.
                     varDsc->lvStkOffs =
                         -initialStkOffs + genMapIntRegNumToRegArgNum(varDsc->GetArgReg()) * REGSIZE_BYTES;
                     continue;
                 }
+
 #endif
 
 #ifdef _TARGET_ARM_

diff --git a/src/coreclr/src/vm/arm64/PInvokeStubs.asm b/src/coreclr/src/vm/arm64/PInvokeStubs.asm
index eb522d9..440af92 100644 (file)
--- a/src/coreclr/src/vm/arm64/PInvokeStubs.asm
+++ b/src/coreclr/src/vm/arm64/PInvokeStubs.asm
@@ -45,11 +45,6 @@ __PInvokeStubFuncName SETS "$FuncPrefix":CC:"Stub"
 __PInvokeGenStubFuncName SETS "$FuncPrefix":CC:"GenILStub"
 __PInvokeStubWorkerName SETS "$FuncPrefix":CC:"StubWorker"
 
-       IF "$VASigCookieReg" == "x1"
-__PInvokeStubFuncName SETS "$__PInvokeStubFuncName":CC:"_RetBuffArg"
-__PInvokeGenStubFuncName SETS "$__PInvokeGenStubFuncName":CC:"_RetBuffArg"
-        ENDIF
-
         NESTED_ENTRY $__PInvokeStubFuncName
 
         ; get the stub
@@ -84,9 +79,7 @@ __PInvokeGenStubFuncName SETS "$__PInvokeGenStubFuncName":CC:"_RetBuffArg"
         mov                 x2, $HiddenArg 
 
         ; x1 = VaSigCookie
-        IF "$VASigCookieReg" != "x1"
         mov                 x1, $VASigCookieReg
-        ENDIF
 
         ; x0 = pTransitionBlock
         add                 x0, sp, #__PWTB_TransitionBlock
@@ -118,7 +111,6 @@ __PInvokeGenStubFuncName SETS "$__PInvokeGenStubFuncName":CC:"_RetBuffArg"
 
 ; ------------------------------------------------------------------
 ; VarargPInvokeStub & VarargPInvokeGenILStub
-; There is a separate stub when the method has a hidden return buffer arg.
 ;
 ; in:
 ; x0 = VASigCookie*
@@ -137,16 +129,6 @@ __PInvokeGenStubFuncName SETS "$__PInvokeGenStubFuncName":CC:"_RetBuffArg"
 ;
         PINVOKE_STUB GenericPInvokeCalli, x15, x12, {true}
 
-; ------------------------------------------------------------------
-; VarargPInvokeStub_RetBuffArg & VarargPInvokeGenILStub_RetBuffArg
-; Vararg PInvoke Stub when the method has a hidden return buffer arg
-;
-; in:
-; x1 = VASigCookie*
-; x12 = MethodDesc*       
-; 
-        PINVOKE_STUB VarargPInvoke, x1, x12, {false}
-
 
 ; Must be at very end of file 
         END

diff --git a/src/coreclr/src/vm/arm64/pinvokestubs.S b/src/coreclr/src/vm/arm64/pinvokestubs.S
index 2ff6a8d..ad64db8 100644 (file)
--- a/src/coreclr/src/vm/arm64/pinvokestubs.S
+++ b/src/coreclr/src/vm/arm64/pinvokestubs.S
@@ -59,9 +59,7 @@ LOCAL_LABEL(\__PInvokeStubFuncName\()_0):
         mov                 x2, \HiddenArg 
 
         // x1 = VaSigCookie
-        .ifnc \VASigCookieReg, x1
         mov                 x1, \VASigCookieReg
-        .endif
 
         // x0 = pTransitionBlock
         add                 x0, sp, #__PWTB_TransitionBlock      
@@ -90,7 +88,6 @@ LOCAL_LABEL(\__PInvokeStubFuncName\()_0):
 
 // ------------------------------------------------------------------
 // VarargPInvokeStub & VarargPInvokeGenILStub
-// There is a separate stub when the method has a hidden return buffer arg.
 //
 // in:
 // x0 = VASigCookie*
@@ -108,13 +105,3 @@ PINVOKE_STUB VarargPInvokeStub, VarargPInvokeGenILStub, VarargPInvokeStubWorker,
 // x12 = Unmanaged target
 //
 PINVOKE_STUB GenericPInvokeCalliHelper, GenericPInvokeCalliGenILStub, GenericPInvokeCalliStubWorker, x15, x12, 1, 1
-
-// ------------------------------------------------------------------
-// VarargPInvokeStub_RetBuffArg & VarargPInvokeGenILStub_RetBuffArg
-// Vararg PInvoke Stub when the method has a hidden return buffer arg
-//
-// in:
-// x1 = VASigCookie*
-// x12 = MethodDesc*       
-// 
-PINVOKE_STUB VarargPInvokeStub_RetBuffArg, VarargPInvokeGenILStub_RetBuffArg, VarargPInvokeStubWorker, x1, x12, 0

diff --git a/src/coreclr/src/vm/prestub.cpp b/src/coreclr/src/vm/prestub.cpp
index 721de3c..ae2b9ac 100644 (file)
--- a/src/coreclr/src/vm/prestub.cpp
+++ b/src/coreclr/src/vm/prestub.cpp
@@ -1990,7 +1990,7 @@ PCODE TheVarargNDirectStub(BOOL hasRetBuffArg)
 {
     LIMITED_METHOD_CONTRACT;
 
-#if !defined(_TARGET_X86_)
+#if !defined(_TARGET_X86_) && !defined(_TARGET_ARM64_)
     if (hasRetBuffArg)
     {
         return GetEEFuncEntryPoint(VarargPInvokeStub_RetBuffArg);

diff --git a/src/coreclr/src/vm/stubmgr.cpp b/src/coreclr/src/vm/stubmgr.cpp
index bc6f47a..2a36908 100644 (file)
--- a/src/coreclr/src/vm/stubmgr.cpp
+++ b/src/coreclr/src/vm/stubmgr.cpp
@@ -1997,7 +1997,7 @@ static BOOL IsVarargPInvokeStub(PCODE stubStartAddress)
     if (stubStartAddress == GetEEFuncEntryPoint(VarargPInvokeStub))
         return TRUE;
 
-#ifndef _TARGET_X86_
+#if !defined(_TARGET_X86_) && !defined(_TARGET_ARM64_)
     if (stubStartAddress == GetEEFuncEntryPoint(VarargPInvokeStub_RetBuffArg))
         return TRUE;
 #endif