From ef56bf314b063ab5baaededf22572603b1d882f3 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 10 Dec 2021 10:33:15 -0800 Subject: [PATCH 1/3] Make sure the combined field size matches the struct size --- src/coreclr/jit/lclvars.cpp | 17 ++++- .../JitBlue/Runtime_62597/Runtime_62597.cs | 71 +++++++++++++++++++ .../Runtime_62597/Runtime_62597.csproj | 12 ++++ 3 files changed, 98 insertions(+), 2 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.csproj diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index debf6bafcfeb5c..970bf4539709be 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -2045,8 +2045,21 @@ bool Compiler::StructPromotionHelper::ShouldPromoteStructVar(unsigned lclNum) // multiple registers? if (compiler->lvaIsMultiregStruct(varDsc, compiler->info.compIsVarArgs)) { - if ((structPromotionInfo.fieldCnt != 2) && - !((structPromotionInfo.fieldCnt == 1) && varTypeIsSIMD(structPromotionInfo.fields[0].fldType))) + if (structPromotionInfo.fieldCnt == 2) + { + unsigned structSize = varDsc->GetLayout()->GetSize(); + if (structPromotionInfo.fields[0].fldSize * 2 != structSize) + { + // Ensure that the combined fldSize matches the structSize otherwise partial information might + // be passed. + JITDUMP("Not promoting multireg struct local V%02u, because lvIsParam is true, #fields == 2, " + "field#0 size == %u, field#1 size == %u, total structSize= %u.\n", + lclNum, structPromotionInfo.fields[0].fldSize, structPromotionInfo.fields[1].fldSize, + structSize); + shouldPromote = false; + } + } + else if (!((structPromotionInfo.fieldCnt == 1) && varTypeIsSIMD(structPromotionInfo.fields[0].fldType))) { JITDUMP("Not promoting multireg struct local V%02u, because lvIsParam is true, #fields != 2 and it's " "not a single SIMD.\n", diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.cs b/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.cs new file mode 100644 index 00000000000000..242155953a05ba --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.cs @@ -0,0 +1,71 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// +// Note: In below test case, we were not honoring the fact that the explicit struct size +// of struct is 32 bytes while the only 2 fields it has is just 2 bytes. In such case, +// we would pass partial struct value. +using System; +using System.Reflection.Emit; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Text; + +[StructLayout(LayoutKind.Explicit, Size = 32)] +public readonly unsafe struct SmallString +{ + [FieldOffset(0)] private readonly byte _length; + [FieldOffset(1)] private readonly byte _firstByte; + + public SmallString(string value) + { + fixed (char* srcPtr = value) + fixed (byte* destPtr = &_firstByte) + { + Encoding.ASCII.GetBytes(srcPtr, value.Length, destPtr, value.Length); + } + + _length = (byte)value.Length; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public byte Dump() + { + fixed (byte* ptr = &_firstByte) + { + byte* next = ptr + 1; + return *next; + } + } +} + +public static class Program +{ + static int result = 0; + public static int Main() + { + var value = new SmallString("foobar"); + Execute(value); + + var method = new DynamicMethod("test", typeof(void), new[] { typeof(SmallString) }, typeof(Program), true); + var il = method.GetILGenerator(); + il.Emit(OpCodes.Ldarg_0); + il.EmitCall(OpCodes.Call, typeof(Program).GetMethod("Execute")!, null); + il.Emit(OpCodes.Pop); + il.Emit(OpCodes.Ret); + + var action = (Action)method.CreateDelegate(typeof(Action)); + action.Invoke(value); + + return result; + } + + public static object Execute(SmallString foo) + { + byte value = foo.Dump(); + if (value == 111) + { + result += 50; + } + return new StringBuilder(); + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.csproj new file mode 100644 index 00000000000000..14d7d03f5c0b65 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.csproj @@ -0,0 +1,12 @@ + + + Exe + + + None + True + + + + + \ No newline at end of file From 674a5aa270ddb712457ef2daf4b0549577a94906 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 10 Dec 2021 11:22:22 -0800 Subject: [PATCH 2/3] Fix the condition --- src/coreclr/jit/lclvars.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 970bf4539709be..eb29090d0cea7a 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -2048,7 +2048,7 @@ bool Compiler::StructPromotionHelper::ShouldPromoteStructVar(unsigned lclNum) if (structPromotionInfo.fieldCnt == 2) { unsigned structSize = varDsc->GetLayout()->GetSize(); - if (structPromotionInfo.fields[0].fldSize * 2 != structSize) + if ((structPromotionInfo.fields[0].fldSize + structPromotionInfo.fields[1].fldSize) != structSize) { // Ensure that the combined fldSize matches the structSize otherwise partial information might // be passed. From f26d8ff2d84b34128739c4704257d60104517f6b Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 10 Dec 2021 14:10:02 -0800 Subject: [PATCH 3/3] Update test and include containHoles condition --- src/coreclr/jit/lclvars.cpp | 18 +++++----------- .../JitBlue/Runtime_62597/Runtime_62597.cs | 21 +++++++++---------- .../Runtime_62597/Runtime_62597.csproj | 1 + 3 files changed, 16 insertions(+), 24 deletions(-) diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index eb29090d0cea7a..4d01e63ef13b6c 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -2045,21 +2045,13 @@ bool Compiler::StructPromotionHelper::ShouldPromoteStructVar(unsigned lclNum) // multiple registers? if (compiler->lvaIsMultiregStruct(varDsc, compiler->info.compIsVarArgs)) { - if (structPromotionInfo.fieldCnt == 2) + if (structPromotionInfo.containsHoles && structPromotionInfo.customLayout) { - unsigned structSize = varDsc->GetLayout()->GetSize(); - if ((structPromotionInfo.fields[0].fldSize + structPromotionInfo.fields[1].fldSize) != structSize) - { - // Ensure that the combined fldSize matches the structSize otherwise partial information might - // be passed. - JITDUMP("Not promoting multireg struct local V%02u, because lvIsParam is true, #fields == 2, " - "field#0 size == %u, field#1 size == %u, total structSize= %u.\n", - lclNum, structPromotionInfo.fields[0].fldSize, structPromotionInfo.fields[1].fldSize, - structSize); - shouldPromote = false; - } + JITDUMP("Not promoting multi-reg struct local V%02u with holes.\n", lclNum); + shouldPromote = false; } - else if (!((structPromotionInfo.fieldCnt == 1) && varTypeIsSIMD(structPromotionInfo.fields[0].fldType))) + else if ((structPromotionInfo.fieldCnt != 2) && + !((structPromotionInfo.fieldCnt == 1) && varTypeIsSIMD(structPromotionInfo.fields[0].fldType))) { JITDUMP("Not promoting multireg struct local V%02u, because lvIsParam is true, #fields != 2 and it's " "not a single SIMD.\n", diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.cs b/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.cs index 242155953a05ba..fb3ccef4ade51b 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.cs @@ -44,27 +44,26 @@ public static class Program public static int Main() { var value = new SmallString("foobar"); - Execute(value); - var method = new DynamicMethod("test", typeof(void), new[] { typeof(SmallString) }, typeof(Program), true); - var il = method.GetILGenerator(); - il.Emit(OpCodes.Ldarg_0); - il.EmitCall(OpCodes.Call, typeof(Program).GetMethod("Execute")!, null); - il.Emit(OpCodes.Pop); - il.Emit(OpCodes.Ret); - - var action = (Action)method.CreateDelegate(typeof(Action)); - action.Invoke(value); + TheTest(value); return result; } + [MethodImpl(MethodImplOptions.NoInlining)] + public static void TheTest(SmallString foo) + { + Execute(foo); + } + + [MethodImpl(MethodImplOptions.NoInlining)] public static object Execute(SmallString foo) { byte value = foo.Dump(); + // 111 corresponds to the ASCII code of 2nd characted of string "foobar" i.e. ASCII value of 'o'. if (value == 111) { - result += 50; + result = 100; } return new StringBuilder(); } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.csproj index 14d7d03f5c0b65..e822a8b10a5a6f 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.csproj +++ b/src/tests/JIT/Regression/JitBlue/Runtime_62597/Runtime_62597.csproj @@ -1,6 +1,7 @@ Exe + True None