Fix handling of Arm32 struct with 8-byte alignment and 12-byte rounded size. (#46320)
authorSergey Andreenko <seandree@microsoft.com>
Wed, 23 Dec 2020 06:43:04 +0000 (22:43 -0800)
committerGitHub <noreply@github.com>
Wed, 23 Dec 2020 06:43:04 +0000 (22:43 -0800)
* Add a repro.

* Fix arm32 struct with 8-byte alignment and 12-byte rounded size.

* format fixes

* review

src/coreclr/jit/compiler.h
src/coreclr/jit/morph.cpp
src/tests/JIT/Regression/JitBlue/Runtime_46239/Runtime_46239.cs [new file with mode: 0644]
src/tests/JIT/Regression/JitBlue/Runtime_46239/Runtime_46239.csproj [new file with mode: 0644]

index 6539b75..2376314 100644 (file)
@@ -1614,7 +1614,6 @@ public:
 
         assert(GetByteSize() > TARGET_POINTER_SIZE * numRegs);
         const unsigned stackByteSize = GetByteSize() - TARGET_POINTER_SIZE * numRegs;
-        assert(IsSplit() || ((stackByteSize % m_byteAlignment) == 0));
         return stackByteSize;
     }
 
@@ -1798,19 +1797,38 @@ public:
         return m_byteOffset;
     }
 
-    void SetByteSize(unsigned byteSize, unsigned byteAlignment)
+    void SetByteSize(unsigned byteSize, bool isStruct, bool isFloatHfa)
     {
-        assert(byteAlignment != 0);
-        const unsigned alignedByteSize = roundUp(byteSize, byteAlignment);
+
+#ifdef OSX_ARM64_ABI
+        unsigned roundedByteSize;
+        // Only struct types need extension or rounding to pointer size, but HFA<float> does not.
+        if (isStruct && !isFloatHfa)
+        {
+            roundedByteSize = roundUp(byteSize, TARGET_POINTER_SIZE);
+        }
+        else
+        {
+            roundedByteSize = byteSize;
+        }
+#else  // OSX_ARM64_ABI
+        unsigned roundedByteSize = roundUp(byteSize, TARGET_POINTER_SIZE);
+#endif // OSX_ARM64_ABI
+
+#if !defined(TARGET_ARM)
+        // Arm32 could have a struct with 8 byte alignment
+        // which rounded size % 8 is not 0.
+        assert(m_byteAlignment != 0);
+        assert(roundedByteSize % m_byteAlignment == 0);
+#endif // TARGET_ARM
 
 #if defined(DEBUG_ARG_SLOTS)
         if (!isStruct)
         {
-            assert(alignedByteSize == getSlotCount() * TARGET_POINTER_SIZE);
+            assert(roundedByteSize == getSlotCount() * TARGET_POINTER_SIZE);
         }
 #endif
-        m_byteSize      = alignedByteSize;
-        m_byteAlignment = byteAlignment;
+        m_byteSize = roundedByteSize;
     }
 
     unsigned GetByteSize() const
@@ -1818,6 +1836,11 @@ public:
         return m_byteSize;
     }
 
+    void SetByteAlignment(unsigned byteAlignment)
+    {
+        m_byteAlignment = byteAlignment;
+    }
+
     unsigned GetByteAlignment() const
     {
         return m_byteAlignment;
@@ -1947,6 +1970,7 @@ public:
                              unsigned          byteSize,
                              unsigned          byteAlignment,
                              bool              isStruct,
+                             bool              isFloatHfa,
                              bool              isVararg = false);
 
 #ifdef UNIX_AMD64_ABI
@@ -1958,6 +1982,7 @@ public:
                              unsigned                                                         byteSize,
                              unsigned                                                         byteAlignment,
                              const bool                                                       isStruct,
+                             const bool                                                       isFloatHfa,
                              const bool                                                       isVararg,
                              const regNumber                                                  otherRegNum,
                              const unsigned                                                   structIntRegs,
@@ -1972,6 +1997,7 @@ public:
                              unsigned          byteSize,
                              unsigned          byteAlignment,
                              bool              isStruct,
+                             bool              isFloatHfa,
                              bool              isVararg = false);
 
     void RemorphReset();
@@ -2500,7 +2526,9 @@ public:
     unsigned ehFuncletCount(); // Return the count of funclets in the function
 
     unsigned bbThrowIndex(BasicBlock* blk); // Get the index to use as the cache key for sharing throw blocks
-#else                                       // !FEATURE_EH_FUNCLETS
+
+#else  // !FEATURE_EH_FUNCLETS
+
     bool ehAnyFunclets()
     {
         return false;
@@ -2514,7 +2542,7 @@ public:
     {
         return blk->bbTryIndex;
     } // Get the index to use as the cache key for sharing throw blocks
-#endif                                      // !FEATURE_EH_FUNCLETS
+#endif // !FEATURE_EH_FUNCLETS
 
     // Returns a flowList representing the "EH predecessors" of "blk".  These are the normal predecessors of
     // "blk", plus one special case: if "blk" is the first block of a handler, considers the predecessor(s) of the first
index 9dc357b..b72d32e 100644 (file)
@@ -991,6 +991,7 @@ fgArgTabEntry* fgArgInfo::AddRegArg(unsigned          argNum,
                                     unsigned          byteSize,
                                     unsigned          byteAlignment,
                                     bool              isStruct,
+                                    bool              isFloatHfa,
                                     bool              isVararg /*=false*/)
 {
     fgArgTabEntry* curArgTabEntry = new (compiler, CMK_fgArgInfo) fgArgTabEntry;
@@ -1026,7 +1027,8 @@ fgArgTabEntry* fgArgInfo::AddRegArg(unsigned          argNum,
     curArgTabEntry->isNonStandard = false;
     curArgTabEntry->isStruct      = isStruct;
     curArgTabEntry->SetIsVararg(isVararg);
-    curArgTabEntry->SetByteSize(byteSize, byteAlignment);
+    curArgTabEntry->SetByteAlignment(byteAlignment);
+    curArgTabEntry->SetByteSize(byteSize, isStruct, isFloatHfa);
     curArgTabEntry->SetByteOffset(0);
 
     hasRegArgs = true;
@@ -1043,6 +1045,7 @@ fgArgTabEntry* fgArgInfo::AddRegArg(unsigned
                                     unsigned                                                         byteSize,
                                     unsigned                                                         byteAlignment,
                                     const bool                                                       isStruct,
+                                    const bool                                                       isFloatHfa,
                                     const bool                                                       isVararg,
                                     const regNumber                                                  otherRegNum,
                                     const unsigned                                                   structIntRegs,
@@ -1050,7 +1053,7 @@ fgArgTabEntry* fgArgInfo::AddRegArg(unsigned
                                     const SYSTEMV_AMD64_CORINFO_STRUCT_REG_PASSING_DESCRIPTOR* const structDescPtr)
 {
     fgArgTabEntry* curArgTabEntry =
-        AddRegArg(argNum, node, use, regNum, numRegs, byteSize, byteAlignment, isStruct, isVararg);
+        AddRegArg(argNum, node, use, regNum, numRegs, byteSize, byteAlignment, isStruct, isFloatHfa, isVararg);
     assert(curArgTabEntry != nullptr);
 
     curArgTabEntry->isStruct        = isStruct; // is this a struct arg
@@ -1080,6 +1083,7 @@ fgArgTabEntry* fgArgInfo::AddStkArg(unsigned          argNum,
                                     unsigned          byteSize,
                                     unsigned          byteAlignment,
                                     bool              isStruct,
+                                    bool              isFloatHfa,
                                     bool              isVararg /*=false*/)
 {
     fgArgTabEntry* curArgTabEntry = new (compiler, CMK_fgArgInfo) fgArgTabEntry;
@@ -1121,14 +1125,14 @@ fgArgTabEntry* fgArgInfo::AddStkArg(unsigned          argNum,
     curArgTabEntry->isStruct      = isStruct;
     curArgTabEntry->SetIsVararg(isVararg);
 
-    curArgTabEntry->SetByteSize(byteSize, byteAlignment);
+    curArgTabEntry->SetByteAlignment(byteAlignment);
+    curArgTabEntry->SetByteSize(byteSize, isStruct, isFloatHfa);
     curArgTabEntry->SetByteOffset(nextStackByteOffset);
 
     hasStackArgs = true;
     AddArg(curArgTabEntry);
     DEBUG_ARG_SLOTS_ONLY(nextSlotNum += numSlots;)
     nextStackByteOffset += curArgTabEntry->GetByteSize();
-    assert(nextStackByteOffset % curArgTabEntry->GetByteAlignment() == 0);
 
     return curArgTabEntry;
 }
@@ -2788,11 +2792,13 @@ void Compiler::fgInitArgInfo(GenTreeCall* call)
         const unsigned  byteSize      = TARGET_POINTER_SIZE;
         const unsigned  byteAlignment = TARGET_POINTER_SIZE;
         const bool      isStruct      = false;
+        const bool      isFloatHfa    = false;
 
         // This is a register argument - put it in the table.
         call->fgArgInfo->AddRegArg(argIndex, argx, call->gtCallThisArg, regNum, numRegs, byteSize, byteAlignment,
-                                   isStruct, callIsVararg UNIX_AMD64_ABI_ONLY_ARG(REG_STK) UNIX_AMD64_ABI_ONLY_ARG(0)
-                                                 UNIX_AMD64_ABI_ONLY_ARG(0) UNIX_AMD64_ABI_ONLY_ARG(nullptr));
+                                   isStruct, isFloatHfa,
+                                   callIsVararg UNIX_AMD64_ABI_ONLY_ARG(REG_STK) UNIX_AMD64_ABI_ONLY_ARG(0)
+                                       UNIX_AMD64_ABI_ONLY_ARG(0) UNIX_AMD64_ABI_ONLY_ARG(nullptr));
 
         intArgRegNum++;
 #ifdef WINDOWS_AMD64_ABI
@@ -2995,6 +3001,8 @@ void Compiler::fgInitArgInfo(GenTreeCall* call)
         }
 #endif // FEATURE_HFA
 
+        const bool isFloatHfa = (hfaType == TYP_FLOAT);
+
 #ifdef TARGET_ARM
         passUsingFloatRegs    = !callIsVararg && (isHfaArg || varTypeUsesFloatReg(argx)) && !opts.compUseSoftFP;
         bool passUsingIntRegs = passUsingFloatRegs ? false : (intArgRegNum < MAX_REG_ARG);
@@ -3219,7 +3227,7 @@ void Compiler::fgInitArgInfo(GenTreeCall* call)
         // Arm64 Apple has a special ABI for passing small size arguments on stack,
         // bytes are aligned to 1-byte, shorts to 2-byte, int/float to 4-byte, etc.
         // It means passing 8 1-byte arguments on stack can take as small as 8 bytes.
-        unsigned argAlignBytes = eeGetArgAlignment(argType, (hfaType == TYP_FLOAT));
+        unsigned argAlignBytes = eeGetArgAlignment(argType, isFloatHfa);
 #endif
 
         //
@@ -3465,12 +3473,12 @@ void Compiler::fgInitArgInfo(GenTreeCall* call)
 #endif
 
             // This is a register argument - put it in the table
-            newArgEntry = call->fgArgInfo->AddRegArg(argIndex, argx, args, nextRegNum, size, byteSize, argAlignBytes,
-                                                     isStructArg, callIsVararg UNIX_AMD64_ABI_ONLY_ARG(nextOtherRegNum)
-                                                                      UNIX_AMD64_ABI_ONLY_ARG(structIntRegs)
-                                                                          UNIX_AMD64_ABI_ONLY_ARG(structFloatRegs)
-                                                                              UNIX_AMD64_ABI_ONLY_ARG(&structDesc));
-
+            newArgEntry =
+                call->fgArgInfo->AddRegArg(argIndex, argx, args, nextRegNum, size, byteSize, argAlignBytes, isStructArg,
+                                           isFloatHfa, callIsVararg UNIX_AMD64_ABI_ONLY_ARG(nextOtherRegNum)
+                                                           UNIX_AMD64_ABI_ONLY_ARG(structIntRegs)
+                                                               UNIX_AMD64_ABI_ONLY_ARG(structFloatRegs)
+                                                                   UNIX_AMD64_ABI_ONLY_ARG(&structDesc));
             newArgEntry->SetIsBackFilled(isBackFilled);
             newArgEntry->isNonStandard = isNonStandard;
 
@@ -3531,7 +3539,7 @@ void Compiler::fgInitArgInfo(GenTreeCall* call)
         {
             // This is a stack argument - put it in the table
             newArgEntry = call->fgArgInfo->AddStkArg(argIndex, argx, args, size, byteSize, argAlignBytes, isStructArg,
-                                                     callIsVararg);
+                                                     isFloatHfa, callIsVararg);
 #ifdef UNIX_AMD64_ABI
             // TODO-Amd64-Unix-CQ: This is temporary (see also in fgMorphArgs).
             if (structDesc.passedInRegisters)
diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_46239/Runtime_46239.cs b/src/tests/JIT/Regression/JitBlue/Runtime_46239/Runtime_46239.cs
new file mode 100644 (file)
index 0000000..983d8ac
--- /dev/null
@@ -0,0 +1,196 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+// The test was reproducing an issue on Arm32 when we required an 8-byte alignment
+// for a struct which size was rounded to 4-byte.
+
+using System;
+using System.Runtime.InteropServices;
+using System.Runtime.CompilerServices;
+
+namespace Runtime_46239
+{
+
+    [StructLayout(LayoutKind.Sequential, Pack = 1)]
+    internal struct S1 // Marshal.SizeOf 12 bytes, EE getClassSize 12 (here and below for arm32).
+    {
+        public ulong tmp1;
+        public Object q;
+    }
+
+    [StructLayout(LayoutKind.Sequential, Pack = 1)]
+    internal struct S2 // Marshal.SizeOf 12 bytes, EE getClassSize 12
+    {
+        public ulong tmp1;
+        public int tmp2;
+    }
+
+    [StructLayout(LayoutKind.Explicit)]
+    internal struct S3 // Marshal.SizeOf 16 bytes, EE getClassSize 12
+    {
+        [FieldOffset(0)]
+        public ulong tmp1;
+        [FieldOffset(8)]
+        public Object tmp2;
+    }
+
+    [StructLayout(LayoutKind.Explicit)]
+    internal struct S4 // Marshal.SizeOf 16 bytes, EE getClassSize 16
+    {
+        [FieldOffset(0)]
+        public ulong tmp1;
+        [FieldOffset(8)]
+        public int tmp2;
+    }
+
+    internal struct S5 // Marshal.SizeOf 16 bytes, EE getClassSize 16
+    {
+        public ulong tmp1;
+        public Object tmp2;
+    }
+
+    internal struct S6 // Marshal.SizeOf 16 bytes, EE getClassSize 16
+    {
+        public ulong tmp1;
+        public int tmp2;
+    }
+
+    class Program
+    {
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static int test1<T>(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, int num, T a, T b)
+        {
+            return 100;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static int test2<T>(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, int num, T a, T b, T c)
+        {
+            return 100;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static int test3<T>(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, byte b1, T a, T b, T c)
+        {
+            return 100;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static int test4<T>(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, T a, T b, T c)
+        {
+            return 100;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static int test5<T>(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, int num, T a, T b, int i)
+        {
+            return 100;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static int test6<T>(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, T a, T b, int i)
+        {
+            return 100;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static int test5<T>(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, byte b1, T a, T b, byte b2)
+        {
+            return 100;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static int test6<T>(int i1, int i2, int i3, int i4, int i5, int i6, int i7, int i8, T a, T b, byte b1)
+        {
+            return 100;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static int test<T>() where T : struct
+        {
+
+            // Marshal.SizeOf throws "System.ArgumentException: Type 'Runtime_46239.S1' cannot 
+            // be marshaled as an unmanaged structure; no meaningful size or offset can be computed."
+            // on non-Windows platforms.
+            //
+            // int size = Marshal.SizeOf(typeof(T));
+            // Console.WriteLine("size of " + typeof(T).Name + " is: " + size);
+
+
+            if (test1<T>(1, 2, 3, 4, 5, 6, 7, 8, 1, new T(), new T()) != 100)
+            {
+                Console.WriteLine("test1() failed.");
+                return 101;
+            }
+            if (test2<T>(1, 2, 3, 4, 5, 6, 7, 8, 1, new T(), new T(), new T()) != 100)
+            {
+                Console.WriteLine("test2() failed.");
+                return 101;
+            }
+            if (test3<T>(1, 2, 3, 4, 5, 6, 7, 8, 1, new T(), new T(), new T()) != 100)
+            {
+                Console.WriteLine("test3() failed.");
+                return 101;
+            }
+            if (test4<T>(1, 2, 3, 4, 5, 6, 7, 8, new T(), new T(), new T()) != 100)
+            {
+                Console.WriteLine("test4() failed.");
+                return 101;
+            }
+            if (test5<T>(1, 2, 3, 4, 5, 6, 7, 8, 1, new T(), new T(), 2) != 100)
+            {
+                Console.WriteLine("test5() failed.");
+                return 101;
+            }
+            if (test6<T>(1, 2, 3, 4, 5, 6, 7, 8, new T(), new T(), 1) != 100)
+            {
+                Console.WriteLine("test6() failed.");
+                return 101;
+            }
+            return 100;
+
+        }
+
+        static int Main(string[] args)
+        {
+
+            if (test<S1>() != 100)
+            {
+                Console.WriteLine("test<S1>() failed.");
+                return 101;
+            }
+
+            if (test<S2>() != 100)
+            {
+                Console.WriteLine("test<S2>() failed.");
+                return 101;
+            }
+
+            if (test<S3>() != 100)
+            {
+                Console.WriteLine("test<S3>() failed.");
+                return 101;
+            }
+
+            if (test<S4>() != 100)
+            {
+                Console.WriteLine("test<S4>() failed.");
+                return 101;
+            }
+
+            if (test<S5>() != 100)
+            {
+                Console.WriteLine("test<S5>() failed.");
+                return 101;
+            }
+
+            if (test<S6>() != 100)
+            {
+                Console.WriteLine("test<S6>() failed.");
+                return 101;
+            }
+
+            return 100;
+        }
+    }
+}
diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_46239/Runtime_46239.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_46239/Runtime_46239.csproj
new file mode 100644 (file)
index 0000000..adc3b72
--- /dev/null
@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <CLRTestPriority>0</CLRTestPriority>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>