From 321217f0f2add9498358aac327742a7daff64386 Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Thu, 3 Jun 2021 21:03:04 -0700 Subject: [PATCH] Retype calls as SIMD when applicable. (#53578) * add a repro * passed spmi. * update comment. * update the test * improve the check. * fix a stressfailure * fix x64 unix diff --- src/coreclr/jit/codegenxarch.cpp | 2 +- src/coreclr/jit/gentree.cpp | 9 +- src/coreclr/jit/importer.cpp | 114 ++++++++------------- .../JitBlue/Runtime_52864/Runtime_52864.cs | 106 +++++++++++++++++++ .../JitBlue/Runtime_52864/Runtime_52864.csproj | 25 +++++ 5 files changed, 176 insertions(+), 80 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_52864/Runtime_52864.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_52864/Runtime_52864.csproj diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index 0fbe764..e830007 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -5414,7 +5414,7 @@ void CodeGen::genCallInstruction(GenTreeCall* call) // A Vector3 return value is stored in xmm0 and xmm1. // RyuJIT assumes that the upper unused bits of xmm1 are cleared but // the native compiler doesn't guarantee it. - if (returnType == TYP_SIMD12) + if (call->IsUnmanaged() && (returnType == TYP_SIMD12)) { returnReg = retTypeDesc->GetABIReturnReg(1); // Clear the upper 32 bits by two shift instructions. diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 1f37457..ca0ef63 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6500,12 +6500,6 @@ GenTree* Compiler::gtNewLclvNode(unsigned lnum, var_types type DEBUGARG(IL_OFFSE // Make an exception for implicit by-ref parameters during global morph, since // their lvType has been updated to byref but their appearances have not yet all // been rewritten and so may have struct type still. - // Also, make an exception for retyping of a lclVar to a SIMD or scalar type of the same - // size as the struct if it has a single field This can happen when we retype the lhs - // of a call assignment. - // TODO-1stClassStructs: When we stop "lying" about the types for ABI purposes, we - // should be able to remove this exception and handle the assignment mismatch in - // Lowering. LclVarDsc* varDsc = lvaGetDesc(lnum); bool simd12ToSimd16Widening = false; @@ -6514,8 +6508,7 @@ GenTree* Compiler::gtNewLclvNode(unsigned lnum, var_types type DEBUGARG(IL_OFFSE simd12ToSimd16Widening = (type == TYP_SIMD16) && (varDsc->lvType == TYP_SIMD12); #endif assert((type == varDsc->lvType) || simd12ToSimd16Widening || - (lvaIsImplicitByRefLocal(lnum) && fgGlobalMorph && (varDsc->lvType == TYP_BYREF)) || - ((varDsc->lvType == TYP_STRUCT) && (genTypeSize(type) == varDsc->lvExactSize))); + (lvaIsImplicitByRefLocal(lnum) && fgGlobalMorph && (varDsc->lvType == TYP_BYREF))); } GenTree* node = new (this, GT_LCL_VAR) GenTreeLclVar(GT_LCL_VAR, type, lnum DEBUGARG(ILoffs)); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 9e3d758..4521a68 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -9489,9 +9489,10 @@ var_types Compiler::impImportJitTestLabelMark(int numArgs) #endif // DEBUG //----------------------------------------------------------------------------------- -// impFixupCallStructReturn: For a call node that returns a struct type either -// adjust the return type to an enregisterable type, or set the flag to indicate -// struct return via retbuf arg. +// impFixupCallStructReturn: For a call node that returns a struct do one of the following: +// - set the flag to indicate struct return via retbuf arg; +// - adjust the return type to a SIMD type if it is returned in 1 reg; +// - spill call result into a temp if it is returned into 2 registers or more and not tail call or inline candidate. // // Arguments: // call - GT_CALL GenTree node @@ -9511,92 +9512,63 @@ GenTree* Compiler::impFixupCallStructReturn(GenTreeCall* call, CORINFO_CLASS_HAN #if FEATURE_MULTIREG_RET call->InitializeStructReturnType(this, retClsHnd, call->GetUnmanagedCallConv()); -#endif // FEATURE_MULTIREG_RET - -#ifdef UNIX_AMD64_ABI - - // Not allowed for FEATURE_CORCLR which is the only SKU available for System V OSs. - assert(!call->IsVarargs() && "varargs not allowed for System V OSs."); - const ReturnTypeDesc* retTypeDesc = call->GetReturnTypeDesc(); const unsigned retRegCount = retTypeDesc->GetReturnRegCount(); - if (retRegCount == 0) - { - // struct not returned in registers i.e returned via hiddden retbuf arg. - call->gtCallMoreFlags |= GTF_CALL_M_RETBUFFARG; - } - else if (retRegCount == 1) - { - return call; - } - else - { - // must be a struct returned in two registers - assert(retRegCount == 2); - - if ((!call->CanTailCall()) && (!call->IsInlineCandidate())) - { - // Force a call returning multi-reg struct to be always of the IR form - // tmp = call - // - // No need to assign a multi-reg struct to a local var if: - // - It is a tail call or - // - The call is marked for in-lining later - return impAssignMultiRegTypeToVar(call, retClsHnd DEBUGARG(call->GetUnmanagedCallConv())); - } - } - -#else // not UNIX_AMD64_ABI +#else // !FEATURE_MULTIREG_RET + const unsigned retRegCount = 1; +#endif // !FEATURE_MULTIREG_RET - // Check for TYP_STRUCT type that wraps a primitive type - // Such structs are returned using a single register - // and we change the return type on those calls here. - // structPassingKind howToReturnStruct; - var_types returnType; - - returnType = getReturnTypeForStruct(retClsHnd, call->GetUnmanagedCallConv(), &howToReturnStruct); + var_types returnType = getReturnTypeForStruct(retClsHnd, call->GetUnmanagedCallConv(), &howToReturnStruct); if (howToReturnStruct == SPK_ByReference) { assert(returnType == TYP_UNKNOWN); call->gtCallMoreFlags |= GTF_CALL_M_RETBUFFARG; + return call; } - else + + // Recognize SIMD types as we do for LCL_VARs, + // note it could be not the ABI specific type, for example, on x64 we can set 'TYP_SIMD8` + // for `System.Numerics.Vector2` here but lower will change it to long as ABI dictates. + var_types simdReturnType = impNormStructType(call->gtRetClsHnd); + if (simdReturnType != call->TypeGet()) { + assert(varTypeIsSIMD(simdReturnType)); + JITDUMP("changing the type of a call [%06u] from %s to %s\n", dspTreeID(call), varTypeName(call->TypeGet()), + varTypeName(returnType)); + call->ChangeType(simdReturnType); + } -#if !FEATURE_MULTIREG_RET + if (retRegCount == 1) + { return call; -#else // FEATURE_MULTIREG_RET - const ReturnTypeDesc* retTypeDesc = call->GetReturnTypeDesc(); - const unsigned retRegCount = retTypeDesc->GetReturnRegCount(); - assert(retRegCount != 0); - if (retRegCount == 1) - { - return call; - } - - assert(returnType == TYP_STRUCT); - assert((howToReturnStruct == SPK_ByValueAsHfa) || (howToReturnStruct == SPK_ByValue)); + } - assert(call->gtReturnType == returnType); +#if FEATURE_MULTIREG_RET + assert(varTypeIsStruct(call)); // It could be a SIMD returned in several regs. + assert(returnType == TYP_STRUCT); + assert((howToReturnStruct == SPK_ByValueAsHfa) || (howToReturnStruct == SPK_ByValue)); - assert(retRegCount >= 2); - if ((!call->CanTailCall()) && (!call->IsInlineCandidate())) - { - // Force a call returning multi-reg struct to be always of the IR form - // tmp = call - // - // No need to assign a multi-reg struct to a local var if: - // - It is a tail call or - // - The call is marked for in-lining later - return impAssignMultiRegTypeToVar(call, retClsHnd DEBUGARG(call->GetUnmanagedCallConv())); - } -#endif // FEATURE_MULTIREG_RET - } +#ifdef UNIX_AMD64_ABI + // must be a struct returned in two registers + assert(retRegCount == 2); +#else // not UNIX_AMD64_ABI + assert(retRegCount >= 2); #endif // not UNIX_AMD64_ABI + if (!call->CanTailCall() && !call->IsInlineCandidate()) + { + // Force a call returning multi-reg struct to be always of the IR form + // tmp = call + // + // No need to assign a multi-reg struct to a local var if: + // - It is a tail call or + // - The call is marked for in-lining later + return impAssignMultiRegTypeToVar(call, retClsHnd DEBUGARG(call->GetUnmanagedCallConv())); + } return call; +#endif // FEATURE_MULTIREG_RET } /***************************************************************************** diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_52864/Runtime_52864.cs b/src/tests/JIT/Regression/JitBlue/Runtime_52864/Runtime_52864.cs new file mode 100644 index 0000000..3834c7a --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_52864/Runtime_52864.cs @@ -0,0 +1,106 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Threading; +using System.Linq; +using System.Runtime.CompilerServices; + +using Point = System.Numerics.Vector2; + +namespace Runtime_52864 +{ + public class test + { + static Point checkA; + static Point checkB; + static Point checkC; + static int returnVal; + + public const int DefaultSeed = 20010415; + public static int Seed = Environment.GetEnvironmentVariable("CORECLR_SEED") switch + { + string seedStr when seedStr.Equals("random", StringComparison.OrdinalIgnoreCase) => new Random().Next(), + string seedStr when int.TryParse(seedStr, out int envSeed) => envSeed, + _ => DefaultSeed + }; + + [MethodImpl(MethodImplOptions.NoInlining)] + static void check(Point a, Point b, Point c) + { + if (a != checkA) + { + Console.WriteLine($"A doesn't match. Should be {checkA} but is {a}"); + returnVal = -1; + } + if (b != checkB) + { + Console.WriteLine($"B doesn't match. Should be {checkB} but is {b}"); + returnVal = -1; + } + if (c != checkC) + { + Console.WriteLine($"C doesn't match. Should be {checkC} but is {c}"); + returnVal = -1; + } + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void FailureCase(List p) + { + Point p1 = p[0]; + Point p2 = p.Last(); + + check(p1, p[1], p2); + check(p1, p[1], p2); + check(p1, p[1], p2); + } + + static Point NextPoint(Random random) + { + return new Point( + (float)random.NextDouble(), + (float)random.NextDouble() + ); + } + + static int Main() + { + returnVal = 100; + Random random = new Random(Seed); + + for (int i = 0; i < 50; i++) + { + List p = new List(); + + checkA = NextPoint(random); + p.Add(checkA); + checkB = NextPoint(random); + p.Add(checkB); + checkC = NextPoint(random); + p.Add(checkC); + + FailureCase(p); + + Thread.Sleep(15); + } + + for (int i = 0; i < 50; i++) + { + List p = new List(); + + checkA = NextPoint(random); + p.Add(checkA); + checkB = NextPoint(random); + p.Add(checkB); + checkC = NextPoint(random); + p.Add(checkC); + + FailureCase(p); + } + + return returnVal; + } + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_52864/Runtime_52864.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_52864/Runtime_52864.csproj new file mode 100644 index 0000000..a898415 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_52864/Runtime_52864.csproj @@ -0,0 +1,25 @@ + + + Exe + 1 + + + None + True + + + + + + + + + -- 2.7.4