From 437a6c3e77e567052aade3727098fad3796925cd Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 3 Jan 2024 10:17:36 +0100 Subject: [PATCH 1/3] JIT: Home float parameters before integer parameters Parameters that are going into float registers can come from integer registers in the presence of struct promotion. We need to home those before integer parameters or the source register could have been overridden by the integer parameter homing logic. Ideally it seems like the homing logic should be unified to handle all parameters simultaneously, but this seems like a simple enough fix. I do not think we have ABIs where we have the opposite kind constraint (integer parameters coming from float registers). Fix #96306 --- src/coreclr/jit/codegencommon.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 72e31c388154ab..c6892747e0548d 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -6154,8 +6154,14 @@ void CodeGen::genFnProlog() }; #if defined(TARGET_AMD64) || defined(TARGET_ARM64) || defined(TARGET_ARM) - assignIncomingRegisterArgs(&intRegState); + // Handle float parameters first; in the presence of struct promotion + // we can have parameters that are homed into float registers but + // passed in integer registers. So make sure we get those out of the + // integer registers before we potentially override those as part of + // handling integer parameters. + assignIncomingRegisterArgs(&floatRegState); + assignIncomingRegisterArgs(&intRegState); #else assignIncomingRegisterArgs(&intRegState); #endif From 1905d52673f52bf5b1579b19281ab5b42ba968e2 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 3 Jan 2024 10:31:09 +0100 Subject: [PATCH 2/3] Add test --- .../JitBlue/Runtime_96306/Runtime_96306.cs | 37 +++++++++++++++++++ .../Runtime_96306/Runtime_96306.csproj | 10 +++++ 2 files changed, 47 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_96306/Runtime_96306.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_96306/Runtime_96306.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_96306/Runtime_96306.cs b/src/tests/JIT/Regression/JitBlue/Runtime_96306/Runtime_96306.cs new file mode 100644 index 00000000000000..043e15ad90522e --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_96306/Runtime_96306.cs @@ -0,0 +1,37 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Numerics; +using System.Runtime.CompilerServices; +using Xunit; + +public class Runtime_96306 +{ + [Fact] + public static int TestEntryPoint() + { + return Foo(new Point2D { V = new Vector2(101, -1) }, 100); + } + + // 'a' is passed in rcx but homed into xmm1 after promotion. + // 'scale' is passed in xmm1 but spilled because of the call to Bar. + // We must take care that we spill 'scale' before we home 'a'. + [MethodImpl(MethodImplOptions.NoInlining)] + private static int Foo(Point2D a, float scale) + { + Bar(); + return ReturnValue(scale); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static int ReturnValue(float value) => (int)value; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void Bar() { } + + private struct Point2D + { + public Vector2 V; + } +} + diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_96306/Runtime_96306.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_96306/Runtime_96306.csproj new file mode 100644 index 00000000000000..6c2e6c34ba4034 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_96306/Runtime_96306.csproj @@ -0,0 +1,10 @@ + + + True + None + true + + + + + \ No newline at end of file From b69690dec7e5149a133b5cbba8e3d85fce064ab5 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 7 Mar 2024 14:03:23 +0100 Subject: [PATCH 3/3] Disable float -> int reg enregistration for some rare cases --- src/coreclr/jit/lsra.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index 0a33ba3faba9bd..c4f2d68e00d708 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -1663,6 +1663,21 @@ bool LinearScan::isRegCandidate(LclVarDsc* varDsc) return false; } + // Avoid allocating parameters that are passed in float regs into integer + // registers. We currently home float registers before integer registers, + // so that kind of enregistration can trash integer registers containing + // other parameters. + // We assume that these cases will be homed to float registers if they are + // promoted. + // TODO-CQ: Combine integer and float register homing to handle these kinds + // of conflicts. + if ((varDsc->TypeGet() == TYP_STRUCT) && varDsc->lvIsRegArg && !varDsc->lvPromoted && + varTypeUsesIntReg(varDsc->GetRegisterType()) && genIsValidFloatReg(varDsc->GetArgReg())) + { + compiler->lvaSetVarDoNotEnregister(lclNum DEBUGARG(DoNotEnregisterReason::IsStructArg)); + return false; + } + // Are we not optimizing and we have exception handlers? // if so mark all args and locals as volatile, so that they // won't ever get enregistered.