From c5fa991c0d14c5b9b879366e2990cf9051eff699 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 20 Jul 2021 11:41:13 +0200 Subject: [PATCH 1/3] Disallow widening for explicit tailcalls It is a runtime detail that the managed calling convention widens return values, so only allow this behavior for opportunistic tailcalls. Fix #55253 --- src/coreclr/jit/compiler.h | 3 +- src/coreclr/jit/importer.cpp | 43 ++++++++++++++----- src/coreclr/jit/morph.cpp | 2 +- .../JitBlue/Runtime_55253/Runtime_55253.cs | 21 +++++++++ .../Runtime_55253/Runtime_55253.csproj | 22 ++++++++++ 5 files changed, 78 insertions(+), 13 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.csproj diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index b34535a6146f9..435b38d251f0c 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4603,7 +4603,8 @@ class Compiler bool exactContextNeedsRuntimeLookup, CORINFO_CALL_INFO* callInfo); - bool impTailCallRetTypeCompatible(var_types callerRetType, + bool impTailCallRetTypeCompatible(bool allowWidening, + var_types callerRetType, CORINFO_CLASS_HANDLE callerRetTypeClass, CorInfoCallConvExtension callerCallConv, var_types calleeRetType, diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 5d6c0d7354f52..0bd23eb475710 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -7863,11 +7863,32 @@ void Compiler::impInsertHelperCall(CORINFO_HELPER_DESC* helperInfo) impAppendTree(callout, (unsigned)CHECK_SPILL_NONE, impCurStmtOffs); } -// Checks whether the return types of caller and callee are compatible -// so that callee can be tail called. Note that here we don't check -// compatibility in IL Verifier sense, but on the lines of return type -// sizes are equal and get returned in the same return register. -bool Compiler::impTailCallRetTypeCompatible(var_types callerRetType, +//------------------------------------------------------------------------ +// impTailCallRetTypeCompatible: Checks whether the return types of caller +// and callee are compatible so that calle can be tail called. +// sizes are not supported integral type sizes return values to temps. +// +// Arguments: +// allowWidening -- whether to allow implicit widening by the callee. +// For instance, allowing int32 -> int16 tailcalls. +// The managed calling convention allows this, but +// we don't want explicit tailcalls to depend on this +// detail of the managed calling convention. +// callerRetType -- the caller's return type +// callerRetTypeClass - the caller's return struct type +// callerCallConv -- calling convention of the caller +// calleeRetType -- the callee's return type +// calleeRetTypeClass - the callee return struct type +// calleeCallConv -- calling convention of the callee +// +// Returns: +// True if the tailcall types are compatible. +// +// Remarks: +// Note that here we don't check compatibility in IL Verifier sense, but on the +// lines of return types getting returned in the same return register. +bool Compiler::impTailCallRetTypeCompatible(bool allowWidening, + var_types callerRetType, CORINFO_CLASS_HANDLE callerRetTypeClass, CorInfoCallConvExtension callerCallConv, var_types calleeRetType, @@ -7886,7 +7907,7 @@ bool Compiler::impTailCallRetTypeCompatible(var_types callerRetTy bool isManaged = (callerCallConv == CorInfoCallConvExtension::Managed) && (calleeCallConv == CorInfoCallConvExtension::Managed); - if (isManaged && varTypeIsIntegral(callerRetType) && varTypeIsIntegral(calleeRetType) && + if (allowWidening && isManaged && varTypeIsIntegral(callerRetType) && varTypeIsIntegral(calleeRetType) && (genTypeSize(callerRetType) <= 4) && (genTypeSize(calleeRetType) <= genTypeSize(callerRetType))) { return true; @@ -9096,12 +9117,12 @@ var_types Compiler::impImportCall(OPCODE opcode, BADCODE("Stack should be empty after tailcall"); } - // Note that we can not relax this condition with genActualType() as - // the calling convention dictates that the caller of a function with - // a small-typed return value is responsible for normalizing the return val - + // For opportunistic tailcalls we allow implicit widening, i.e. tailcalls from int32 -> int16, since the + // managed calling convention dictates that the callee widens the value. For explicit tailcalls we don't + // want to require this detail of the calling convention to bubble up to the tailcall helpers + bool allowWidening = isImplicitTailCall; if (canTailCall && - !impTailCallRetTypeCompatible(info.compRetType, info.compMethodInfo->args.retTypeClass, info.compCallConv, + !impTailCallRetTypeCompatible(allowWidening, info.compRetType, info.compMethodInfo->args.retTypeClass, info.compCallConv, callRetTyp, sig->retTypeClass, call->AsCall()->GetUnmanagedCallConv())) { canTailCall = false; diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 057c318e81dcf..1ab0f2a050ef4 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -6872,7 +6872,7 @@ bool Compiler::fgCanFastTailCall(GenTreeCall* callee, const char** failReason) if (callee->IsTailPrefixedCall()) { var_types retType = info.compRetType; - assert(impTailCallRetTypeCompatible(retType, info.compMethodInfo->args.retTypeClass, info.compCallConv, + assert(impTailCallRetTypeCompatible(false, retType, info.compMethodInfo->args.retTypeClass, info.compCallConv, (var_types)callee->gtReturnType, callee->gtRetClsHnd, callee->GetUnmanagedCallConv())); } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.cs b/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.cs new file mode 100644 index 0000000000000..7296a52b92a56 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.cs @@ -0,0 +1,21 @@ +class Runtime_55253 +{ + static int Main() + { + int errors = 0; + if (AsInt32() != -1) + errors |= 1; + if (AsUInt32() != 255) + errors |= 2; + + return 100 + errors; + } + + static uint AsUInt32() => AsUInt16(); + static uint AsUInt16() => AsUInt8(); + static uint AsUInt8() => 255; + + static int AsInt32() => AsInt16(); + static short AsInt16() => AsInt8(); + static sbyte AsInt8() => -1; +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.csproj new file mode 100644 index 0000000000000..94bfaab314cf6 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.csproj @@ -0,0 +1,22 @@ + + + Exe + + + None + False + + + + + + + + + \ No newline at end of file From d72fdcfedbb1db68c17e1f3e86658ce23cf87d76 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 20 Jul 2021 11:45:24 +0200 Subject: [PATCH 2/3] Run jit-format --- src/coreclr/jit/importer.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 0bd23eb475710..f6ec7f3824166 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -9122,8 +9122,9 @@ var_types Compiler::impImportCall(OPCODE opcode, // want to require this detail of the calling convention to bubble up to the tailcall helpers bool allowWidening = isImplicitTailCall; if (canTailCall && - !impTailCallRetTypeCompatible(allowWidening, info.compRetType, info.compMethodInfo->args.retTypeClass, info.compCallConv, - callRetTyp, sig->retTypeClass, call->AsCall()->GetUnmanagedCallConv())) + !impTailCallRetTypeCompatible(allowWidening, info.compRetType, info.compMethodInfo->args.retTypeClass, + info.compCallConv, callRetTyp, sig->retTypeClass, + call->AsCall()->GetUnmanagedCallConv())) { canTailCall = false; szCanTailCallFailReason = "Return types are not tail call compatible"; From b20467752cdc6e7ed56d8f7c11bda74d8dd01d12 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 20 Jul 2021 11:58:44 +0200 Subject: [PATCH 3/3] Add missing license headers --- .../JIT/Regression/JitBlue/Runtime_54842/Runtime_54842.cs | 3 +++ .../JIT/Regression/JitBlue/Runtime_55140/Runtime_55140.cs | 3 +++ .../JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.cs | 3 +++ 3 files changed, 9 insertions(+) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_54842/Runtime_54842.cs b/src/tests/JIT/Regression/JitBlue/Runtime_54842/Runtime_54842.cs index 8941e1c2d59e8..c5d607d23f269 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_54842/Runtime_54842.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_54842/Runtime_54842.cs @@ -1,3 +1,6 @@ +// 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.Runtime.CompilerServices; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_55140/Runtime_55140.cs b/src/tests/JIT/Regression/JitBlue/Runtime_55140/Runtime_55140.cs index 220d40aa87750..3c22e543902cb 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_55140/Runtime_55140.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_55140/Runtime_55140.cs @@ -1,3 +1,6 @@ +// 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.Runtime.CompilerServices; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.cs b/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.cs index 7296a52b92a56..bccdf99b8f3b0 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_55253/Runtime_55253.cs @@ -1,3 +1,6 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + class Runtime_55253 { static int Main()