Fix ARM's genPutArgStk codegen (#23867)

* Fix ARM's genPutArgStk codegen

When the OBJ node wraps a LCL_VAR node the code uses the type information (struct size, GC layout) from LclVarDsc. This is not always correct because the OBJ may actually have a different struct type due to type reinterpretation (e.g. Unsafe.As<X, Y>).

* Fix genPutArgStk comment

diff --git a/src/jit/codegenarmarch.cpp b/src/jit/codegenarmarch.cpp
index add7b98..66d5b22 100644 (file)
--- a/src/jit/codegenarmarch.cpp
+++ b/src/jit/codegenarmarch.cpp
@@ -714,7 +714,7 @@ void CodeGen::genPutArgStk(GenTreePutArgStk* treeNode)
             BYTE* gcPtrs                        = gcPtrArray;
 
             unsigned gcPtrCount; // The count of GC pointers in the struct
-            int      structSize;
+            unsigned structSize;
             bool     isHfa;
 
             // This is the varNum for our load operations,
@@ -764,12 +764,40 @@ void CodeGen::genPutArgStk(GenTreePutArgStk* treeNode)
                     hiReg = addrReg;
                 }
 #endif // _TARGET_ARM64_
+            }
 
+            if (source->OperIs(GT_OBJ))
+            {
+                // If the source is an OBJ node then we need to use the type information
+                // it provides (size and GC layout) even if the node wraps a lclvar. Due
+                // to struct reinterpretation (e.g. Unsafe.As<X, Y>) it is possible that
+                // the OBJ node has a different type than the lclvar.
                 CORINFO_CLASS_HANDLE objClass = source->gtObj.gtClass;
 
                 structSize = compiler->info.compCompHnd->getClassSize(objClass);
-                isHfa      = compiler->IsHfa(objClass);
+
+                // The codegen code below doesn't have proper support for struct sizes
+                // that are not multiple of the slot size. Call arg morphing handles this
+                // case by copying non-local values to temporary local variables.
+                // More generally, we can always round up the struct size when the OBJ node
+                // wraps a local variable because the local variable stack allocation size
+                // is also rounded up to be a multiple of the slot size.
+                if (varNode != nullptr)
+                {
+                    structSize = roundUp(structSize, TARGET_POINTER_SIZE);
+                }
+                else
+                {
+                    assert((structSize % TARGET_POINTER_SIZE) == 0);
+                }
+
+                isHfa = compiler->IsHfa(objClass);
+
 #ifdef _TARGET_ARM64_
+                // On ARM32, Lowering places the correct GC layout information in the
+                // GenTreePutArgStk node and the code above already use that. On ARM64,
+                // this information is not available (in order to keep GenTreePutArgStk
+                // nodes small) and we need to retrieve it from the VM here.
                 gcPtrCount = compiler->info.compCompHnd->getClassGClayout(objClass, &gcPtrs[0]);
 #endif
             }

diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_23794/GitHub_23794.cs b/tests/src/JIT/Regression/JitBlue/GitHub_23794/GitHub_23794.cs
new file mode 100644 (file)
index 0000000..bfa6fcc
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_23794/GitHub_23794.cs
@@ -0,0 +1,58 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
+
+class Program
+{
+    [StructLayout(LayoutKind.Sequential)]
+    struct S
+    {
+        public uint i0;
+        public uint i1;
+        public uint i2;
+        public uint i3;
+
+        public int i4;
+        public int i5;
+    }
+
+    [StructLayout(LayoutKind.Sequential)]
+    struct S16
+    {
+        public uint i0;
+        public uint i1;
+        public uint i2;
+        public uint i3;
+    }
+
+    static int Main()
+    {
+        S s = new S();
+        s.i0 = 0x12345678;
+        s.i1 = 0x87654321;
+        return Test(s);
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static int Call(int r0, int r1, int r2, int r3, int r4, int r5, int r6, S16 s)
+    {
+        return (s.i0 == 0x12345678 && s.i1 == 0x87654321) ? 100 : 1;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static void Escape<T>(ref T t)
+    {
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static int Test(S p)
+    {
+        S s = p;
+        Escape(ref s);
+        return Call(0, 1, 2, 3, 4, 5, 6, Unsafe.As<S, S16>(ref s));
+    }
+}

diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_23794/GitHub_23794.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_23794/GitHub_23794.csproj
new file mode 100644 (file)
index 0000000..83594da
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_23794/GitHub_23794.csproj
@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Release</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <OutputType>Exe</OutputType>
+    <DebugType></DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>