From 0166a97723e4ed929fcb70315aaa0c8916083473 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 1 Jun 2022 13:34:23 +0200 Subject: [PATCH 1/4] JIT ARM32: Fix odd sized structs from arbitrary sources ARM32 ABI allows passing structs in register even when their sizes are not divisible by 4. This means we sometimes need to pass 3 bytes in the last register. The JIT would not handle this when the source was an arbitrary memory location (this would require multiple loads and shifts). The fix is to just force a copy into the local stack frame for this case. Fix #61168 --- src/coreclr/jit/morph.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index e36feb309892e6..dea9055bf37ae9 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -3270,6 +3270,17 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call) makeOutArgCopy = true; } + // ARM32 has an edge case where we need to pass 3 bytes in + // the last register, which would require two loads and a + // shift to avoid loading too much if the source is a + // potentially arbitrary address. Handle the edge case here + // by copying into the local stack frame first so we can + // use a register wide load. + if (!arg.AbiInfo.IsSplit() && (((arg.AbiInfo.NumRegs * REGSIZE_BYTES) - passingSize) == 1)) + { + makeOutArgCopy = true; + } + if (structSize < TARGET_POINTER_SIZE) { makeOutArgCopy = true; @@ -3854,7 +3865,8 @@ GenTree* Compiler::fgMorphMultiregStructArg(CallArg* arg) break; #endif // (TARGET_ARM64) || (UNIX_AMD64_ABI) || (TARGET_LOONGARCH64) default: - noway_assert(!"NYI: odd sized struct in fgMorphMultiregStructArg"); + noway_assert(!"Cannot load odd sized last element from arbitrary source; expected source to be " + "local in this case"); break; } } From 77648cc5238a709d3bcf091f7d0a96e7c7c4a191 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 1 Jun 2022 13:38:10 +0200 Subject: [PATCH 2/4] Reenable ABI tests Fix #68837 Fix #70042 --- src/tests/issues.targets | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/tests/issues.targets b/src/tests/issues.targets index 47fb27b300a780..0637d41ba60411 100644 --- a/src/tests/issues.targets +++ b/src/tests/issues.targets @@ -543,12 +543,6 @@ https://github.com/dotnet/runtime/issues/66745 - - https://github.com/dotnet/runtime/issues/70042 - - - https://github.com/dotnet/runtime/issues/70042 - @@ -678,18 +672,6 @@ https://github.com/dotnet/runtime/issues/57856 - - https://github.com/dotnet/runtime/issues/68837 - - - https://github.com/dotnet/runtime/issues/68837 - - - https://github.com/dotnet/runtime/issues/68837 - - - https://github.com/dotnet/runtime/issues/68837 - https://github.com/dotnet/runtime/issues/57875 From 03013e0592c70a9b69616b5a295d60865932b891 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 1 Jun 2022 13:46:22 +0200 Subject: [PATCH 3/4] Add test --- .../JIT/Directed/StructABI/SevenByteStruct.cs | 35 +++++++++++++++++++ .../Directed/StructABI/SevenByteStruct.csproj | 12 +++++++ 2 files changed, 47 insertions(+) create mode 100644 src/tests/JIT/Directed/StructABI/SevenByteStruct.cs create mode 100644 src/tests/JIT/Directed/StructABI/SevenByteStruct.csproj diff --git a/src/tests/JIT/Directed/StructABI/SevenByteStruct.cs b/src/tests/JIT/Directed/StructABI/SevenByteStruct.cs new file mode 100644 index 00000000000000..b8924b99bc417f --- /dev/null +++ b/src/tests/JIT/Directed/StructABI/SevenByteStruct.cs @@ -0,0 +1,35 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; + +// On ARM32 the following has S0 passed in two registers, which requires passing 3 bytes in the last register. +// We cannot do that in a single load from an arbitrary source and must copy it to a local first. + +public struct S0 +{ + public byte F0; + public byte F1; + public byte F2; + public byte F3; + public byte F4; + public byte F5; + public byte F6; +} + +public class SevenByteStruct +{ + public static S0 s_4 = new S0 { F0 = 1, F1 = 2, F2 = 3, F3 = 4, F4 = 5, F5 = 6, F6 = 7 }; + public static int Main() + { + ref S0 vr0 = ref s_4; + int sum = M35(vr0); + return sum == 28 ? 100 : -1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static int M35(S0 arg0) + { + return arg0.F0 + arg0.F1 + arg0.F2 + arg0.F3 + arg0.F4 + arg0.F5 + arg0.F6; + } +} diff --git a/src/tests/JIT/Directed/StructABI/SevenByteStruct.csproj b/src/tests/JIT/Directed/StructABI/SevenByteStruct.csproj new file mode 100644 index 00000000000000..9d281a449d0710 --- /dev/null +++ b/src/tests/JIT/Directed/StructABI/SevenByteStruct.csproj @@ -0,0 +1,12 @@ + + + Exe + + + PdbOnly + True + + + + + From 34df355c7d2760fb0ab52a494f1053f72facc729 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 3 Jun 2022 12:05:27 +0200 Subject: [PATCH 4/4] Fix for split args --- src/coreclr/jit/morph.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index dea9055bf37ae9..fb5932bb6e6aac 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -3270,13 +3270,14 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call) makeOutArgCopy = true; } - // ARM32 has an edge case where we need to pass 3 bytes in - // the last register, which would require two loads and a - // shift to avoid loading too much if the source is a - // potentially arbitrary address. Handle the edge case here - // by copying into the local stack frame first so we can - // use a register wide load. - if (!arg.AbiInfo.IsSplit() && (((arg.AbiInfo.NumRegs * REGSIZE_BYTES) - passingSize) == 1)) + // If the arg may go into registers (both fully or split) + // then we cannot handle passing it from an arbitrary + // source if it would require passing a 3 byte chunk. + // Placing 3 bytes into a register requires multiple loads/shifts/or. + // In theory we could more easily support it for split args + // as those load registers fully always, but currently we + // do not. + if ((arg.AbiInfo.NumRegs > 0) && ((passingSize % REGSIZE_BYTES) == 3)) { makeOutArgCopy = true; } @@ -3865,8 +3866,7 @@ GenTree* Compiler::fgMorphMultiregStructArg(CallArg* arg) break; #endif // (TARGET_ARM64) || (UNIX_AMD64_ABI) || (TARGET_LOONGARCH64) default: - noway_assert(!"Cannot load odd sized last element from arbitrary source; expected source to be " - "local in this case"); + noway_assert(!"Cannot load odd sized last element from arbitrary source"); break; } }