From 960be870f8e67214c574eba1c6f7f5872fb29ad0 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 2 Sep 2021 23:01:55 +0200 Subject: [PATCH 1/7] Add cast when replacing promoted struct with field in lowering We may need to sign/zero-extend when we do this replacement very late. Normally this cast would be inserted in morph. Fix #58373 --- src/coreclr/jit/lower.cpp | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index ef0578b2cbff50..5dfaa390ca1b09 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3408,6 +3408,7 @@ void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret) unsigned lclNum = lclVar->GetLclNum(); LclVarDsc* varDsc = comp->lvaGetDesc(lclNum); + bool replacedInLowering = false; if (varDsc->CanBeReplacedWithItsField(comp)) { // We can replace the struct with its only field and keep the field on a register. @@ -3422,6 +3423,7 @@ void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret) lclVar->ChangeType(fieldDsc->lvType); lclNum = fieldLclNum; varDsc = comp->lvaGetDesc(lclNum); + replacedInLowering = true; } else if (varDsc->lvPromoted) { @@ -3432,6 +3434,7 @@ void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret) if (varDsc->lvDoNotEnregister) { + assert(!replacedInLowering); lclVar->ChangeOper(GT_LCL_FLD); lclVar->AsLclFld()->SetLclOffs(0); lclVar->ChangeType(ret->TypeGet()); @@ -3440,12 +3443,23 @@ void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret) { const var_types lclVarType = varDsc->GetRegisterType(lclVar); assert(lclVarType != TYP_UNDEF); + + if (varDsc->lvNormalizeOnLoad() && replacedInLowering) + { + // For a normalize-on-load var that we replaced late we need to insert a cast + // as morph would typically be responsible for this. + GenTreeCast* cast = comp->gtNewCastNode(TYP_INT, lclVar, false, lclVarType); + ret->gtOp1 = cast; + BlockRange().InsertBefore(ret, cast); + ContainCheckCast(cast); + } + const var_types actualType = genActualType(lclVarType); lclVar->ChangeType(actualType); if (varTypeUsesFloatReg(ret) != varTypeUsesFloatReg(lclVarType)) { - GenTree* bitcast = comp->gtNewBitCastNode(ret->TypeGet(), lclVar); + GenTree* bitcast = comp->gtNewBitCastNode(ret->TypeGet(), ret->gtOp1); ret->gtOp1 = bitcast; BlockRange().InsertBefore(ret, bitcast); ContainCheckBitCast(bitcast); From aabdb1200507602f691736dda12c46de65170dc2 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 2 Sep 2021 23:08:24 +0200 Subject: [PATCH 2/7] Add test --- .../JitBlue/Runtime_58373/Runtime_58373.cs | 29 +++++++++++++++++++ .../Runtime_58373/Runtime_58373.csproj | 13 +++++++++ 2 files changed, 42 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.cs b/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.cs new file mode 100644 index 00000000000000..213ef2d9265088 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.cs @@ -0,0 +1,29 @@ +// 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; + +public unsafe class Runtime_58373 +{ + public static int Main() + { + FillStack(0, 0, 0, 0, 0, 0, 0xdeadbeef); + short val1 = HalfToInt16Bits(0, 0, 0, 0, 0, 0, (Half)42f); + FillStack(0, 0, 0, 0, 0, 0, 0xf000baaa); + short val2 = HalfToInt16Bits(0, 0, 0, 0, 0, 0, (Half)42f); + + return val1 == val2 ? 100 : -1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static void FillStack(int a0, int a1, int a2, int a3, int a4, int a5, uint onStack) + { + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static short HalfToInt16Bits(int a0, int a1, int a2, int a3, int a4, int a5, Half h) + { + return *(short*)&h; + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.csproj new file mode 100644 index 00000000000000..1a1d3eadae5ce4 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.csproj @@ -0,0 +1,13 @@ + + + Exe + + + None + True + True + + + + + \ No newline at end of file From 7a55f1e8e6de7a93a2e319de13c2875c74ec9830 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 2 Sep 2021 23:54:17 +0200 Subject: [PATCH 3/7] Fix formatting --- src/coreclr/jit/lower.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 5dfaa390ca1b09..16409252b1749e 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3421,8 +3421,8 @@ void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret) "[%06u]\n", lclNum, fieldLclNum, comp->dspTreeID(ret)); lclVar->ChangeType(fieldDsc->lvType); - lclNum = fieldLclNum; - varDsc = comp->lvaGetDesc(lclNum); + lclNum = fieldLclNum; + varDsc = comp->lvaGetDesc(lclNum); replacedInLowering = true; } else if (varDsc->lvPromoted) @@ -3449,7 +3449,7 @@ void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret) // For a normalize-on-load var that we replaced late we need to insert a cast // as morph would typically be responsible for this. GenTreeCast* cast = comp->gtNewCastNode(TYP_INT, lclVar, false, lclVarType); - ret->gtOp1 = cast; + ret->gtOp1 = cast; BlockRange().InsertBefore(ret, cast); ContainCheckCast(cast); } From 48c8474cfc95a7710ec131b3b8aa8687b8482c08 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 3 Sep 2021 12:17:51 +0200 Subject: [PATCH 4/7] Fix test cases --- .../JitBlue/Runtime_58373/Runtime_58373.cs | 29 ------------ .../JitBlue/Runtime_58373/Runtime_58373_1.cs | 29 ++++++++++++ ...me_58373.csproj => Runtime_58373_1.csproj} | 0 .../JitBlue/Runtime_58373/Runtime_58373_2.cs | 47 +++++++++++++++++++ .../Runtime_58373/Runtime_58373_2.csproj | 13 +++++ 5 files changed, 89 insertions(+), 29 deletions(-) delete mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_1.cs rename src/tests/JIT/Regression/JitBlue/Runtime_58373/{Runtime_58373.csproj => Runtime_58373_1.csproj} (100%) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_2.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_2.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.cs b/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.cs deleted file mode 100644 index 213ef2d9265088..00000000000000 --- a/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.cs +++ /dev/null @@ -1,29 +0,0 @@ -// 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; - -public unsafe class Runtime_58373 -{ - public static int Main() - { - FillStack(0, 0, 0, 0, 0, 0, 0xdeadbeef); - short val1 = HalfToInt16Bits(0, 0, 0, 0, 0, 0, (Half)42f); - FillStack(0, 0, 0, 0, 0, 0, 0xf000baaa); - short val2 = HalfToInt16Bits(0, 0, 0, 0, 0, 0, (Half)42f); - - return val1 == val2 ? 100 : -1; - } - - [MethodImpl(MethodImplOptions.NoInlining)] - static void FillStack(int a0, int a1, int a2, int a3, int a4, int a5, uint onStack) - { - } - - [MethodImpl(MethodImplOptions.NoInlining)] - static short HalfToInt16Bits(int a0, int a1, int a2, int a3, int a4, int a5, Half h) - { - return *(short*)&h; - } -} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_1.cs b/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_1.cs new file mode 100644 index 00000000000000..76a46072e556d2 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_1.cs @@ -0,0 +1,29 @@ +// 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; + +public unsafe class Runtime_58373 +{ + public static int Main() + { + short halfValue = HalfToInt16Bits(MakeHalf()); + int x = halfValue; + short val2 = HalfToInt16Bits(*(Half*)&x); + + return halfValue == val2 ? 100 : -1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static Half MakeHalf() + { + return (Half)(-1.0f); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static short HalfToInt16Bits(Half h) + { + return *(short*)&h; + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_1.csproj similarity index 100% rename from src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373.csproj rename to src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_1.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_2.cs b/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_2.cs new file mode 100644 index 00000000000000..c8d1286c1ba7b2 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_2.cs @@ -0,0 +1,47 @@ +// 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; + +public unsafe class Runtime_58373 +{ + public static int Main() + { + // Use up a lot of registers + int a = GetVal(); + int b = GetVal(); + int c = GetVal(); + int d = GetVal(); + int e = GetVal(); + int f = GetVal(); + int g = GetVal(); + int h = GetVal(); + int i = GetVal(); + + short val1 = HalfToInt16Bits(MakeHalf()); + Half half = MakeHalf(); + MakeHalf(); // This will spill lower 16 bits of 'half' to memory + short val2 = HalfToInt16Bits(half); // This will pass 32 bits as arg with upper 16 bits undefined + + return val1 == val2 ? 100 + a + b + c + d + e + f + g + h + i : -1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int GetVal() + { + return 0; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static Half MakeHalf() + { + return default; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static short HalfToInt16Bits(Half h) + { + return *(short*)&h; + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_2.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_2.csproj new file mode 100644 index 00000000000000..1a1d3eadae5ce4 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_58373/Runtime_58373_2.csproj @@ -0,0 +1,13 @@ + + + Exe + + + None + True + True + + + + + \ No newline at end of file From 5ce3a0f4c517b3c0fa158c0b716054459a99d60b Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Sep 2021 14:55:07 +0200 Subject: [PATCH 5/7] Type returned-struct-as-primitive correctly for normalization Retyping it to the type of the ret node is not right when reinterpreting small structs as a type that needs to be normalized. --- src/coreclr/jit/lower.cpp | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 16409252b1749e..30f76ce6a8cd03 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2984,9 +2984,8 @@ void Lowering::LowerRet(GenTreeUnOp* ret) // There are two kinds of retyping: // - A simple bitcast can be inserted when: // - We're returning a floating type as an integral type or vice-versa, or - // - We're returning a struct as a primitive type and using the old form of retyping. - // - If we're returning a struct as a primitive type and *not* using old retying, we change the type of - // 'retval' in 'LowerRetStructLclVar()' + // - If we're returning a struct as a primitive type, we change the type of + // 'retval' in 'LowerRetStructLclVar()' bool needBitcast = (ret->TypeGet() != TYP_VOID) && (varTypeUsesFloatReg(ret) != varTypeUsesFloatReg(ret->gtGetOp1())); bool doPrimitiveBitcast = false; @@ -3437,7 +3436,26 @@ void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret) assert(!replacedInLowering); lclVar->ChangeOper(GT_LCL_FLD); lclVar->AsLclFld()->SetLclOffs(0); - lclVar->ChangeType(ret->TypeGet()); + + // We are returning as a primitive type and the lcl is of struct type. + assert(!varTypeIsStruct(comp->info.compRetNativeType)); + assert((genTypeSize(comp->info.compRetNativeType) == genTypeSize(ret)) || + (varTypeIsIntegral(ret) && varTypeIsIntegral(comp->info.compRetNativeType) && (genTypeSize(comp->info.compRetNativeType) <= genTypeSize(ret)))); + // If the actual return type requires normalization, then make sure we + // do so by using the correct small type for the GT_LCL_FLD. It would + // be conservative to check just compRetNativeType for this since small + // structs are normalized to primitive types when they are returned in + // registers, so we would normalize for them as well. + if (varTypeIsSmall(comp->info.compRetType)) + { + assert(genTypeSize(comp->info.compRetNativeType) == genTypeSize(comp->info.compRetType)); + lclVar->ChangeType(comp->info.compRetType); + } + else + { + // Otherwise we don't mind that we leave the upper bits undefined. + lclVar->ChangeType(ret->TypeGet()); + } } else { From 72d0c9d6ffd1bdba256cb473f5a7de446681b8ee Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Sep 2021 19:20:56 +0200 Subject: [PATCH 6/7] Fix assertion --- src/coreclr/jit/lower.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 30f76ce6a8cd03..75fc6e23fc6783 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3438,7 +3438,7 @@ void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret) lclVar->AsLclFld()->SetLclOffs(0); // We are returning as a primitive type and the lcl is of struct type. - assert(!varTypeIsStruct(comp->info.compRetNativeType)); + assert(comp->info.compRetNativeType != TYP_STRUCT); assert((genTypeSize(comp->info.compRetNativeType) == genTypeSize(ret)) || (varTypeIsIntegral(ret) && varTypeIsIntegral(comp->info.compRetNativeType) && (genTypeSize(comp->info.compRetNativeType) <= genTypeSize(ret)))); // If the actual return type requires normalization, then make sure we From 525071956a735015eaf74686971a5d991727a575 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Sep 2021 19:32:13 +0200 Subject: [PATCH 7/7] Run jit-format --- src/coreclr/jit/lower.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 75fc6e23fc6783..b0c76482db9b88 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3440,7 +3440,8 @@ void Lowering::LowerRetSingleRegStructLclVar(GenTreeUnOp* ret) // We are returning as a primitive type and the lcl is of struct type. assert(comp->info.compRetNativeType != TYP_STRUCT); assert((genTypeSize(comp->info.compRetNativeType) == genTypeSize(ret)) || - (varTypeIsIntegral(ret) && varTypeIsIntegral(comp->info.compRetNativeType) && (genTypeSize(comp->info.compRetNativeType) <= genTypeSize(ret)))); + (varTypeIsIntegral(ret) && varTypeIsIntegral(comp->info.compRetNativeType) && + (genTypeSize(comp->info.compRetNativeType) <= genTypeSize(ret)))); // If the actual return type requires normalization, then make sure we // do so by using the correct small type for the GT_LCL_FLD. It would // be conservative to check just compRetNativeType for this since small