[RISC-V][LoongArch64] Pass FP struct fields at arbitrary offsets in ArgIterator and CallDescrWorker

src/coreclr/tools/Common/Internal/Runtime/RiscVLoongArch64FpStruct.cs

@@ -9,37 +9,6 @@
 
 namespace Internal.JitInterface
 {
-    // StructFloatFieldInfoFlags: used on LoongArch64 and RISC-V architecture as a legacy representation of
-    // FpStructInRegistersInfo, returned by FpStructInRegistersInfo.ToOldFlags()
-    //
-    // `STRUCT_NO_FLOAT_FIELD` means structs are not passed using the float register(s).
-    //
-    // Otherwise, and only for structs with no more than two fields and a total struct size no larger
-    // than two pointers:
-    //
-    // The lowest four bits denote the floating-point info:
-    //   bit 0: `1` means there is only one float or double field within the struct.
-    //   bit 1: `1` means only the first field is floating-point type.
-    //   bit 2: `1` means only the second field is floating-point type.
-    //   bit 3: `1` means the two fields are both floating-point type.
-    // The bits[5:4] denoting whether the field size is 8-bytes:
-    //   bit 4: `1` means the first field's size is 8.
-    //   bit 5: `1` means the second field's size is 8.
-    //
-    // Note that bit 0 and 3 cannot both be set.
-    [Flags]
-    public enum StructFloatFieldInfoFlags
-    {
-        STRUCT_NO_FLOAT_FIELD         = 0x0,
-        STRUCT_FLOAT_FIELD_ONLY_ONE   = 0x1,
-        STRUCT_FLOAT_FIELD_ONLY_TWO   = 0x8,
-        STRUCT_FLOAT_FIELD_FIRST      = 0x2,
-        STRUCT_FLOAT_FIELD_SECOND     = 0x4,
-        STRUCT_FIRST_FIELD_SIZE_IS8   = 0x10,
-        STRUCT_SECOND_FIELD_SIZE_IS8  = 0x20,
-    };
-
-
     // Bitfields for FpStructInRegistersInfo.flags
     [Flags]
     public enum FpStruct
@@ -79,17 +48,6 @@ public struct FpStructInRegistersInfo
 
         public uint Size1st() { return 1u << (int)SizeShift1st(); }
         public uint Size2nd() { return 1u << (int)SizeShift2nd(); }
-
-        public StructFloatFieldInfoFlags ToOldFlags()
-        {
-            return
-                ((flags & FpStruct.OnlyOne) != 0 ? StructFloatFieldInfoFlags.STRUCT_FLOAT_FIELD_ONLY_ONE : 0) |
-                ((flags & FpStruct.BothFloat) != 0 ? StructFloatFieldInfoFlags.STRUCT_FLOAT_FIELD_ONLY_TWO : 0) |
-                ((flags & FpStruct.FloatInt) != 0 ? StructFloatFieldInfoFlags.STRUCT_FLOAT_FIELD_FIRST : 0) |
-                ((flags & FpStruct.IntFloat) != 0 ? StructFloatFieldInfoFlags.STRUCT_FLOAT_FIELD_SECOND : 0) |
-                ((SizeShift1st() == 3) ? StructFloatFieldInfoFlags.STRUCT_FIRST_FIELD_SIZE_IS8 : 0) |
-                ((SizeShift2nd() == 3) ? StructFloatFieldInfoFlags.STRUCT_SECOND_FIELD_SIZE_IS8 : 0);
-        }
     }
 
     internal static class RiscVLoongArch64FpStruct

src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TransitionBlock.cs

@@ -303,10 +303,12 @@ public virtual bool IsVarArgPassedByRef(int size)
             return size > EnregisteredParamTypeMaxSize;
         }
 
-        public void ComputeReturnValueTreatment(CorElementType type, TypeHandle thRetType, bool isVarArgMethod, out bool usesRetBuffer, out uint fpReturnSize)
+        public void ComputeReturnValueTreatment(CorElementType type, TypeHandle thRetType, bool isVarArgMethod, out bool usesRetBuffer, out uint fpReturnSize, out uint returnedFpFieldOffset1st, out uint returnedFpFieldOffset2nd)
         {
             usesRetBuffer = false;
             fpReturnSize = 0;
+            returnedFpFieldOffset1st = 0;
+            returnedFpFieldOffset2nd = 0;
 
             switch (type)
             {
@@ -397,8 +399,13 @@ public void ComputeReturnValueTreatment(CorElementType type, TypeHandle thRetTyp
                             if (size <= EnregisteredReturnTypeIntegerMaxSize)
                             {
                                 if (IsLoongArch64 || IsRiscV64)
-                                    fpReturnSize = (uint)RiscVLoongArch64FpStruct.GetFpStructInRegistersInfo(
-                                        thRetType.GetRuntimeTypeHandle(), Architecture).flags;
+                                {
+                                    FpStructInRegistersInfo info = RiscVLoongArch64FpStruct.GetFpStructInRegistersInfo(
+                                        thRetType.GetRuntimeTypeHandle(), Architecture);
+                                    fpReturnSize = (uint)info.flags;
+                                    returnedFpFieldOffset1st = info.offset1st;
+                                    returnedFpFieldOffset2nd = info.offset2nd;
+                                }
                                 break;
                             }
 

src/coreclr/vm/argdestination.h

@@ -106,76 +106,54 @@ class ArgDestination
         #endif // TARGET_RISCV64
 
         _ASSERTE(IsStructPassedInRegs());
+        _ASSERTE(destOffset == 0);
         _ASSERTE(fieldBytes <= 16);
 
-        int argOfs = TransitionBlock::GetOffsetOfFloatArgumentRegisters() + m_argLocDescForStructInRegs->m_idxFloatReg * 8;
-
-        static const FpStruct::Flags twoFloats = FpStruct::Flags(FpStruct::BothFloat
-            | (2 << FpStruct::PosSizeShift1st)
-            | (2 << FpStruct::PosSizeShift2nd));
-        if (m_argLocDescForStructInRegs->m_structFields.flags == twoFloats)
-        { // struct with two floats.
-            _ASSERTE(m_argLocDescForStructInRegs->m_cFloatReg == 2);
-            _ASSERTE(m_argLocDescForStructInRegs->m_cGenReg == 0);
-            *(INT64*)((char*)m_base + argOfs) = NanBox | *(INT32*)src;
-            *(INT64*)((char*)m_base + argOfs + 8) = NanBox | *((INT32*)src + 1);
-        }
-        else if ((m_argLocDescForStructInRegs->m_structFields.flags & FpStruct::FloatInt) != 0)
-        { // the first field is float or double.
-            _ASSERTE(m_argLocDescForStructInRegs->m_cFloatReg == 1);
-            _ASSERTE(m_argLocDescForStructInRegs->m_cGenReg == 1);
-            _ASSERTE((m_argLocDescForStructInRegs->m_structFields.flags & FpStruct::IntFloat) == 0);//the second field is integer.
+        using namespace FpStruct;
+        FpStructInRegistersInfo info = m_argLocDescForStructInRegs->m_structFields;
+        _ASSERTE(m_argLocDescForStructInRegs->m_cFloatReg == ((info.flags & BothFloat) ? 2 : 1));
+        _ASSERTE(m_argLocDescForStructInRegs->m_cGenReg == ((info.flags & (FloatInt | IntFloat)) ? 1 : 0));
+        _ASSERTE(info.offset2nd + info.Size2nd() <= fieldBytes);
 
-            if (m_argLocDescForStructInRegs->m_structFields.SizeShift1st() == 3)
-            {
-                *(INT64*)((char*)m_base + argOfs) = NanBox | *(INT32*)src; // the first field is float
-            }
-            else
-            {
-                *(UINT64*)((char*)m_base + argOfs) = *(UINT64*)src; // the first field is double.
-            }
+        int floatRegOffset = TransitionBlock::GetOffsetOfFloatArgumentRegisters() +
+            m_argLocDescForStructInRegs->m_idxFloatReg * FLOAT_REGISTER_SIZE;
+        INT64* floatReg = (INT64*)((char*)m_base + floatRegOffset);
+        static_assert(sizeof(*floatReg) == FLOAT_REGISTER_SIZE, "");
 
-            argOfs = TransitionBlock::GetOffsetOfArgumentRegisters() + m_argLocDescForStructInRegs->m_idxGenReg * 8;
-            if (m_argLocDescForStructInRegs->m_structFields.SizeShift1st() == 3 ||
-                m_argLocDescForStructInRegs->m_structFields.SizeShift2nd() == 3)
-            {
-                *(UINT64*)((char*)m_base + argOfs) = *((UINT64*)src + 1);
-            }
-            else
-            {
-                *(INT64*)((char*)m_base + argOfs) = *((INT32*)src + 1); // the second field is int32.
-            }
+        if (info.flags & (OnlyOne | BothFloat | FloatInt)) // copy first floating field
+        {
+            void* field = (char*)src + info.offset1st;
+            *floatReg++ = (info.SizeShift1st() == 3) ? *(INT64*)field : NanBox | *(INT32*)field;
         }
-        else if ((m_argLocDescForStructInRegs->m_structFields.flags & FpStruct::IntFloat) != 0)
-        { // the second field is float or double.
-            _ASSERTE(m_argLocDescForStructInRegs->m_cFloatReg == 1);
-            _ASSERTE(m_argLocDescForStructInRegs->m_cGenReg == 1);
-            _ASSERTE((m_argLocDescForStructInRegs->m_structFields.flags & FpStruct::FloatInt) == 0);//the first field is integer.
-
-            // destOffset - nonzero when copying values into Nullable<T>, it is the offset of the T value inside of the Nullable<T>.
-            // here the first field maybe Nullable.
-            if (m_argLocDescForStructInRegs->m_structFields.SizeShift1st() < 3 &&
-                m_argLocDescForStructInRegs->m_structFields.SizeShift2nd() < 3)
-            {
-                // the second field is float.
-                *(INT64*)((char*)m_base + argOfs) = NanBox | (destOffset == 0 ? *((INT32*)src + 1) : *(INT32*)src);
-            }
-            else
-            {
-                // the second field is double.
-                *(UINT64*)((char*)m_base + argOfs) = destOffset == 0 ? *((UINT64*)src + 1) : *(UINT64*)src;
-            }
 
-            if (0 == destOffset)
-            {
-                // NOTE: here ignoring the first size.
-                argOfs = TransitionBlock::GetOffsetOfArgumentRegisters() + m_argLocDescForStructInRegs->m_idxGenReg * 8;
-                *(UINT64*)((char*)m_base + argOfs) = *(UINT64*)src;
-            }
+        if (info.flags & (BothFloat | IntFloat)) // copy second floating field
+        {
+            void* field = (char*)src + info.offset2nd;
+            *floatReg = (info.SizeShift2nd() == 3) ? *(INT64*)field : NanBox | *(INT32*)field;
         }
-        else
+
+        if (info.flags & (FloatInt | IntFloat)) // copy integer field
         {
-            _ASSERTE(!"---------UNReachable-------LoongArch64/RISC-V64!!!");
+            int intRegOffset = TransitionBlock::GetOffsetOfArgumentRegisters() +
+                m_argLocDescForStructInRegs->m_idxGenReg * TARGET_POINTER_SIZE;
+            void* intReg = (char*)m_base + intRegOffset;
+
+            // Unlike passing primitives on RISC-V, the integer field of a struct passed by hardware floating-point
+            // calling convention is not type-extended to full register length. Trash the upper bits so the callee
+            // accidentally assuming it is extended consistently gets a bad value.
+            RISCV64_ONLY(INDEBUG(*(INT64*)intReg = 0xDadAddedC0ffee00l;))
+
+            uint32_t offset = (info.flags & IntFloat) ? info.offset1st : info.offset2nd;
+            void* field = (char*)src + offset;
+            unsigned sizeShift = (info.flags & IntFloat) ? info.SizeShift1st() : info.SizeShift2nd();
+            switch (sizeShift)
+            {
+                case 0: *(INT8* )intReg = *(INT8* )field; break;
+                case 1: *(INT16*)intReg = *(INT16*)field; break;
+                case 2: *(INT32*)intReg = *(INT32*)field; break;
+                case 3: *(INT64*)intReg = *(INT64*)field; break;
+                default: _ASSERTE(false);
+            }
         }
     }
 

src/coreclr/vm/callhelpers.cpp

@@ -158,6 +158,37 @@ void DispatchCallDebuggerWrapper(
     PAL_ENDTRY
 }
 
+#if defined(TARGET_RISCV64) || defined(TARGET_LOONGARCH64)
+void CopyReturnedFpStructFromRegisters(void* dest, UINT64 returnRegs[2], FpStructInRegistersInfo info,
+    bool handleGcRefs)
+{
+    _ASSERTE(info.flags != FpStruct::UseIntCallConv);
+
+    auto copyReg = [handleGcRefs, dest, returnRegs](uint32_t destOffset, unsigned regIndex, bool isInt, unsigned sizeShift)
+    {
+        const UINT64* srcField = &returnRegs[regIndex];
+        void* destField = (char*)dest + destOffset;
+        int size = 1 << sizeShift;
+
+        static const int ptrShift = 3;
+        static_assert((1 << ptrShift) == TARGET_POINTER_SIZE, "");
+        bool maybeRef = handleGcRefs && isInt && sizeShift == ptrShift && (destOffset & ((1 << ptrShift) - 1)) == 0;
+
+        if (maybeRef)
+            memmoveGCRefs(destField, srcField, size);
+        else
+            memcpyNoGCRefs(destField, srcField, size);
+    };
+
+    // returnRegs contain [ fa0, fa1/a0 ]; FpStruct::IntFloat is the only case where the field order is swapped
+    bool swap = info.flags & FpStruct::IntFloat;
+
+    copyReg(info.offset1st, (swap ? 1 : 0), (info.flags & FpStruct::IntFloat), info.SizeShift1st());
+    if ((info.flags & FpStruct::OnlyOne) == 0)
+        copyReg(info.offset2nd, (swap ? 0 : 1), (info.flags & FpStruct::FloatInt), info.SizeShift2nd());
+}
+#endif // TARGET_RISCV64 || TARGET_LOONGARCH64
+
 // Helper for VM->managed calls with simple signatures.
 void * DispatchCallSimple(
                     SIZE_T *pSrc,
@@ -539,13 +570,7 @@ void MethodDescCallSite::CallTargetWorker(const ARG_SLOT *pArguments, ARG_SLOT *
 #ifdef CALLDESCR_REGTYPEMAP
     callDescrData.dwRegTypeMap = dwRegTypeMap;
 #endif
-#if defined(TARGET_RISCV64) || defined(TARGET_LOONGARCH64)
-    // Temporary conversion to old flags, CallDescrWorker needs to be overhauled anyway
-    // to work with arbitrary field offsets and sizes, and support struct size > 16 on RISC-V.
-    callDescrData.fpReturnSize = FpStructInRegistersInfo{FpStruct::Flags(fpReturnSize)}.ToOldFlags();
-#else
     callDescrData.fpReturnSize = fpReturnSize;
-#endif
     callDescrData.pTarget = m_pCallTarget;
 
 #ifdef FEATURE_INTERPRETER
@@ -560,15 +585,27 @@ void MethodDescCallSite::CallTargetWorker(const ARG_SLOT *pArguments, ARG_SLOT *
         CallDescrWorkerWithHandler(&callDescrData);
     }
 
+#ifdef FEATURE_HFA
     if (pvRetBuff != NULL)
     {
         memcpyNoGCRefs(pvRetBuff, &callDescrData.returnValue, sizeof(callDescrData.returnValue));
     }
+#endif // FEATURE_HFA
 
     if (pReturnValue != NULL)
     {
         _ASSERTE((DWORD)cbReturnValue <= sizeof(callDescrData.returnValue));
-        memcpyNoGCRefs(pReturnValue, &callDescrData.returnValue, cbReturnValue);
+#if defined(TARGET_RISCV64) || defined(TARGET_LOONGARCH64)
+        if (callDescrData.fpReturnSize != FpStruct::UseIntCallConv)
+        {
+            FpStructInRegistersInfo info = m_argIt.GetReturnFpStructInRegistersInfo();
+            CopyReturnedFpStructFromRegisters(pReturnValue, callDescrData.returnValue, info, false);
+        }
+        else
+#endif // defined(TARGET_RISCV64) || defined(TARGET_LOONGARCH64)
+        {
+            memcpyNoGCRefs(pReturnValue, &callDescrData.returnValue, cbReturnValue);
+        }
 
 #if !defined(HOST_64BIT) && BIGENDIAN
         {

src/coreclr/vm/callhelpers.h

@@ -73,6 +73,13 @@ void * DispatchCallSimple(
                     PCODE pTargetAddress,
                     DWORD dwDispatchCallSimpleFlags);
 
+#if defined(TARGET_RISCV64) || defined(TARGET_LOONGARCH64)
+// Copy structs returned according to floating-point calling convention from 'returnRegs' containing struct fields
+// (each returned in one register) as they are filled by CallDescrWorkerInternal, to the final destination in memory
+// 'dest' respecting the struct's layout described in 'info'.
+void CopyReturnedFpStructFromRegisters(void* dest, UINT64 returnRegs[2], FpStructInRegistersInfo info, bool handleGcRefs);
+#endif // defined(TARGET_RISCV64) || defined(TARGET_LOONGARCH64)
+
 bool IsCerRootMethod(MethodDesc *pMD);
 
 class MethodDescCallSite