From 672a925466e174e4e43347e8c224bbe941465691 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 23 May 2023 14:18:14 +0200 Subject: [PATCH 1/2] JIT: Fix constrained call dereferences happening too early MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Constrained calls in IL include an implicit dereference that occurs as part of the call. The JIT was adding this dereference on top of the 'this' argument tree, which makes it happen too early (before other arguments are evaluated). This changes the importer to spill when necessary to preserve ordering. For: ```cil .method private hidebysig static void Foo(class Runtime_73615/C arg) cil managed noinlining { // Code size 21 (0x15) .maxstack 2 IL_0000: ldarga.s arg IL_0002: ldarga.s arg IL_0004: call int32 Runtime_73615::Bar(class Runtime_73615/C&) IL_0009: constrained. Runtime_73615/C IL_000f: callvirt instance void Runtime_73615/C::Baz(int32) IL_0014: ret } ``` Before: ``` ***** BB01 STMT00000 ( 0x000[E-] ... 0x014 ) [000003] --C-G------ ▌ CALL nullcheck void Runtime_73615+C:Baz(int):this [000004] n---G------ this ├──▌ IND ref [000000] ----------- │ └──▌ LCL_ADDR long V00 arg0 [+0] [000002] --C-G------ arg1 └──▌ CALL int Runtime_73615:Bar(byref):int [000001] ----------- arg0 └──▌ LCL_ADDR byref V00 arg0 [+0] ``` After: ``` ***** BB01 STMT00000 ( 0x000[E-] ... 0x014 ) [000005] -AC-G------ ▌ ASG int [000004] D------N--- ├──▌ LCL_VAR int V02 tmp1 [000002] --C-G------ └──▌ CALL int Runtime_73615:Bar(byref):int [000001] ----------- arg0 └──▌ LCL_ADDR byref V00 arg0 [+0] ***** BB01 STMT00001 ( ??? ... ??? ) [000003] --C-G------ ▌ CALL nullcheck void Runtime_73615+C:Baz(int):this [000007] n---G------ this ├──▌ IND ref [000000] ----------- │ └──▌ LCL_ADDR long V00 arg0 [+0] [000006] ----------- arg1 └──▌ LCL_VAR int V02 tmp1 ``` Fix #73615 --- src/coreclr/jit/importercalls.cpp | 19 +++- .../JitBlue/Runtime_73615/Runtime_73615.cs | 60 ++++++++++ .../JitBlue/Runtime_73615/Runtime_73615.il | 105 ++++++++++++++++++ .../Runtime_73615/Runtime_73615.ilproj | 8 ++ 4 files changed, 189 insertions(+), 3 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.il create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.ilproj diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 61af557d583c29..715a5d9afc9253 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -879,7 +879,21 @@ var_types Compiler::impImportCall(OPCODE opcode, } //------------------------------------------------------------------------- - // The main group of arguments + // The main group of arguments, and the this pointer. + + // 'this' is pushed on the IL stack before all call args, but if this is a + // constrained call 'this' is a byref that may need to be dereferenced. + // That dereference should happen _after_ all args, so we need to spill + // them if they can interfere. + bool hasThis; + hasThis = ((mflags & CORINFO_FLG_STATIC) == 0) && ((sig->callConv & CORINFO_CALLCONV_EXPLICITTHIS) == 0) && + ((opcode != CEE_NEWOBJ) || (newobjThis != nullptr)); + + if (hasThis && (constraintCallThisTransform == CORINFO_DEREF_THIS)) + { + impSpillSideEffects(false, CHECK_SPILL_ALL DEBUGARG( + "constrained call requires dereference for 'this' right before call")); + } impPopCallArgs(sig, call->AsCall()); if (extraArg.Node != nullptr) @@ -899,8 +913,7 @@ var_types Compiler::impImportCall(OPCODE opcode, //------------------------------------------------------------------------- // The "this" pointer - if (((mflags & CORINFO_FLG_STATIC) == 0) && ((sig->callConv & CORINFO_CALLCONV_EXPLICITTHIS) == 0) && - !((opcode == CEE_NEWOBJ) && (newobjThis == nullptr))) + if (hasThis) { GenTree* obj; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.cs b/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.cs new file mode 100644 index 00000000000000..de07bfda241972 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.cs @@ -0,0 +1,60 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Reference source for Runtime_73615.il. + +using InlineIL; +using System; +using System.Runtime.CompilerServices; +using Xunit; + +public class Runtime_73615 +{ + [Fact] + public static int TestEntryPoint() + { + Foo(new C(101)); + + if (Result == 100) + { + Console.WriteLine("PASS"); + } + else + { + Console.WriteLine("FAIL: Got result {0}", Result); + } + + return Result; + } + + public static int Result; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void Foo(C arg) + { + IL.Emit.Ldarga(nameof(arg)); + IL.Emit.Ldarga(nameof(arg)); + IL.Emit.Call(new MethodRef(typeof(Runtime_73615), nameof(Bar))); + IL.Emit.Constrained(); + IL.Emit.Callvirt(new MethodRef(typeof(C), nameof(arg.Baz))); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int Bar(ref C o) + { + o = new C(100); + return 0; + } + + public class C + { + public C(int result) => Result = result; + public int Result; + + [MethodImpl(MethodImplOptions.NoInlining)] + public void Baz(int arg) + { + Runtime_73615.Result = Result; + } + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.il b/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.il new file mode 100644 index 00000000000000..26f2ba71a2a9e9 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.il @@ -0,0 +1,105 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +.assembly extern System.Runtime { .publickeytoken = (B0 3F 5F 7F 11 D5 0A 3A ) } +.assembly extern xunit.core { } +.assembly extern System.Console { } + +.assembly Runtime_73615 { } + +.class public auto ansi beforefieldinit Runtime_73615 + extends [System.Runtime]System.Object +{ + .class auto ansi nested public beforefieldinit C + extends [System.Runtime]System.Object + { + .field public int32 Result + .method public hidebysig specialname rtspecialname + instance void .ctor(int32 result) cil managed + { + // Code size 14 (0xe) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: call instance void [System.Runtime]System.Object::.ctor() + IL_0006: ldarg.0 + IL_0007: ldarg.1 + IL_0008: stfld int32 Runtime_73615/C::Result + IL_000d: ret + } // end of method C::.ctor + + .method public hidebysig instance void + Baz(int32 arg) cil managed noinlining + { + // Code size 12 (0xc) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: ldfld int32 Runtime_73615/C::Result + IL_0006: stsfld int32 Runtime_73615::Result + IL_000b: ret + } // end of method C::Baz + + } // end of class C + + .field public static int32 Result + .method public hidebysig static int32 Main() cil managed + { + .custom instance void [xunit.core]Xunit.FactAttribute::.ctor() = ( 01 00 00 00 ) + .entrypoint + // Code size 59 (0x3b) + .maxstack 8 + IL_0000: ldc.i4.s 101 + IL_0002: newobj instance void Runtime_73615/C::.ctor(int32) + IL_0007: call void Runtime_73615::Foo(class Runtime_73615/C) + IL_000c: ldsfld int32 Runtime_73615::Result + IL_0011: ldc.i4.s 100 + IL_0013: bne.un.s IL_0021 + + IL_0015: ldstr "PASS" + IL_001a: call void [System.Console]System.Console::WriteLine(string) + IL_001f: br.s IL_0035 + + IL_0021: ldstr "FAIL: Got result {0}" + IL_0026: ldsfld int32 Runtime_73615::Result + IL_002b: box [System.Runtime]System.Int32 + IL_0030: call void [System.Console]System.Console::WriteLine(string, + object) + IL_0035: ldsfld int32 Runtime_73615::Result + IL_003a: ret + } // end of method Runtime_73615::TestEntryPoint + + .method private hidebysig static void Foo(class Runtime_73615/C arg) cil managed noinlining + { + // Code size 21 (0x15) + .maxstack 2 + IL_0000: ldarga.s arg + IL_0002: ldarga.s arg + IL_0004: call int32 Runtime_73615::Bar(class Runtime_73615/C&) + IL_0009: constrained. Runtime_73615/C + IL_000f: callvirt instance void Runtime_73615/C::Baz(int32) + IL_0014: ret + } // end of method Runtime_73615::Foo + + .method private hidebysig static int32 + Bar(class Runtime_73615/C& o) cil managed noinlining + { + // Code size 11 (0xb) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: ldc.i4.s 100 + IL_0003: newobj instance void Runtime_73615/C::.ctor(int32) + IL_0008: stind.ref + IL_0009: ldc.i4.0 + IL_000a: ret + } // end of method Runtime_73615::Bar + + .method public hidebysig specialname rtspecialname + instance void .ctor() cil managed + { + // Code size 7 (0x7) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: call instance void [System.Runtime]System.Object::.ctor() + IL_0006: ret + } // end of method Runtime_73615::.ctor + +} // end of class Runtime_73615 diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.ilproj b/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.ilproj new file mode 100644 index 00000000000000..5fa250452852d2 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.ilproj @@ -0,0 +1,8 @@ + + + True + + + + + From 7aacf25cea88f8061e513dee9bad589c0dfaea45 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 15 Apr 2024 10:50:17 +0200 Subject: [PATCH 2/2] Address feedback --- .../JitBlue/Runtime_73615/Runtime_73615.cs | 10 ---------- .../JitBlue/Runtime_73615/Runtime_73615.il | 17 ++--------------- 2 files changed, 2 insertions(+), 25 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.cs b/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.cs index de07bfda241972..ccca99fb956cbb 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.cs @@ -14,16 +14,6 @@ public class Runtime_73615 public static int TestEntryPoint() { Foo(new C(101)); - - if (Result == 100) - { - Console.WriteLine("PASS"); - } - else - { - Console.WriteLine("FAIL: Got result {0}", Result); - } - return Result; } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.il b/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.il index 26f2ba71a2a9e9..261d8f25f84b35 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.il +++ b/src/tests/JIT/Regression/JitBlue/Runtime_73615/Runtime_73615.il @@ -45,26 +45,13 @@ { .custom instance void [xunit.core]Xunit.FactAttribute::.ctor() = ( 01 00 00 00 ) .entrypoint - // Code size 59 (0x3b) + // Code size 18 (0x12) .maxstack 8 IL_0000: ldc.i4.s 101 IL_0002: newobj instance void Runtime_73615/C::.ctor(int32) IL_0007: call void Runtime_73615::Foo(class Runtime_73615/C) IL_000c: ldsfld int32 Runtime_73615::Result - IL_0011: ldc.i4.s 100 - IL_0013: bne.un.s IL_0021 - - IL_0015: ldstr "PASS" - IL_001a: call void [System.Console]System.Console::WriteLine(string) - IL_001f: br.s IL_0035 - - IL_0021: ldstr "FAIL: Got result {0}" - IL_0026: ldsfld int32 Runtime_73615::Result - IL_002b: box [System.Runtime]System.Int32 - IL_0030: call void [System.Console]System.Console::WriteLine(string, - object) - IL_0035: ldsfld int32 Runtime_73615::Result - IL_003a: ret + IL_0011: ret } // end of method Runtime_73615::TestEntryPoint .method private hidebysig static void Foo(class Runtime_73615/C arg) cil managed noinlining