From 546cc65f34ce93b00d0ad415163465336276991f Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Wed, 4 Nov 2020 16:40:47 -0800 Subject: [PATCH 1/6] Add a repro test. --- .../JitBlue/Runtime_44266/Runtime_44266.il | 55 +++++++++++++++++++ .../Runtime_44266/Runtime_44266.ilproj | 12 ++++ 2 files changed, 67 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.ilproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il b/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il new file mode 100644 index 00000000000000..44c1e247ec5218 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il @@ -0,0 +1,55 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// This test shows an inlining of `byref LCL_VAR_ADDR - byref CNST_INT` method that returns a native int. +// However, Jit could try to optimize `-` as `+ -CNST_INT` that could lead to an incorrect `long + (-byref)`. + +.assembly extern System.Console {} +.assembly extern legacy library mscorlib {} +.assembly byrefsubbyref1 { } +.class a extends [mscorlib]System.Object +{ + .field static class ctest S_1 + .method public static native int byrefsubbyref(class ctest& V_1, class ctest& V_2) aggressiveinlining + { + ldarg 0 + ldarg 1 + sub + ret + } + + .method public static int32 main() cil managed + { + .entrypoint + .maxstack 2 + .locals init (class ctest V_1, + class ctest V_2, + native int V_3) + newobj instance void ctest::.ctor() + stloc.0 + newobj instance void ctest::.ctor() + dup + stsfld class ctest a::S_1 + stloc.1 + + ldloca V_1 + ldsflda class ctest a::S_1 + call native int a::byrefsubbyref(class ctest&, class ctest&) + stloc V_3 + ldloc V_3 + ret + } +} + +.class private auto ansi ctest + extends [mscorlib]System.Object +{ + .method public specialname rtspecialname + instance void .ctor() cil managed + { + .maxstack 1 + ldarg.0 + call instance void [mscorlib]System.Object::.ctor() + ret + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.ilproj b/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.ilproj new file mode 100644 index 00000000000000..c12a73d5048aa6 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.ilproj @@ -0,0 +1,12 @@ + + + Exe + + + + True + + + + + From 660dad6de52f9ad643e39c9de37fcb93a5bf07c3 Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Wed, 4 Nov 2020 16:45:37 -0800 Subject: [PATCH 2/6] Forbid the transformation for byrefs. --- src/coreclr/src/jit/morph.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index d74e851be899eb..8eb4194ff60b01 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -13354,9 +13354,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) /* Check for "op1 - cns2" , we change it to "op1 + (-cns2)" */ noway_assert(op2); - if (op2->IsCnsIntOrI()) + if (op2->IsCnsIntOrI() && varTypeIsIntOrI(op2)) { - /* Negate the constant and change the node to be "+" */ + // Negate the constant and change the node to be "+", + // except when `op2` is a const byref. op2->AsIntConCommon()->SetIconValue(-op2->AsIntConCommon()->IconValue()); op2->AsIntConRef().gtFieldSeq = FieldSeqStore::NotAField(); From db8b3ff2a325984996dd7660d4c3d8ea19c77f74 Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Wed, 4 Nov 2020 16:09:08 -1000 Subject: [PATCH 3/6] Update src/coreclr/src/jit/morph.cpp Co-authored-by: Andy Ayers --- src/coreclr/src/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 8eb4194ff60b01..016e26d86e3e38 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -13354,7 +13354,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) /* Check for "op1 - cns2" , we change it to "op1 + (-cns2)" */ noway_assert(op2); - if (op2->IsCnsIntOrI() && varTypeIsIntOrI(op2)) + if (op2->IsCnsIntOrI() && !op2->IsIConHandle()) { // Negate the constant and change the node to be "+", // except when `op2` is a const byref. From 63188875a88e62336d51b471406026013d8b72c4 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 4 Nov 2020 19:42:56 -0800 Subject: [PATCH 4/6] Update src/coreclr/src/jit/morph.cpp --- src/coreclr/src/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 016e26d86e3e38..e26fc7d250b9a8 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -13354,7 +13354,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) /* Check for "op1 - cns2" , we change it to "op1 + (-cns2)" */ noway_assert(op2); - if (op2->IsCnsIntOrI() && !op2->IsIConHandle()) + if (op2->IsCnsIntOrI() && !op2->IsIconHandle()) { // Negate the constant and change the node to be "+", // except when `op2` is a const byref. From 8d1d5d43a27ee840d509be7a27e5572dc715727c Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Thu, 5 Nov 2020 02:13:53 -0800 Subject: [PATCH 5/6] Fix the test return value. WriteLine is just to make sure we don't delete the value. --- .../JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il | 2 ++ .../JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.ilproj | 4 +--- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il b/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il index 44c1e247ec5218..18d2fdd8fe7988 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il +++ b/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il @@ -37,6 +37,8 @@ call native int a::byrefsubbyref(class ctest&, class ctest&) stloc V_3 ldloc V_3 + call void [System.Console]System.Console::WriteLine(int32) + ldc.i4.s 100 ret } } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.ilproj b/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.ilproj index c12a73d5048aa6..7dab57fe6d2256 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.ilproj +++ b/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.ilproj @@ -1,9 +1,7 @@ Exe - - - + None True From f81f6c9d68ca85a79df323ced62b1ae3a141e603 Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Thu, 5 Nov 2020 13:09:47 -0800 Subject: [PATCH 6/6] improve the test. avoid a possible overflow and don't waste time on printing. --- .../JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il b/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il index 18d2fdd8fe7988..b51e8988b70d13 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il +++ b/src/tests/JIT/Regression/JitBlue/Runtime_44266/Runtime_44266.il @@ -37,10 +37,16 @@ call native int a::byrefsubbyref(class ctest&, class ctest&) stloc V_3 ldloc V_3 - call void [System.Console]System.Console::WriteLine(int32) + call void a::KeepA(native int) ldc.i4.s 100 ret } + + .method public hidebysig static void KeepA(native int a) cil managed noinlining + { + .maxstack 8 + ret + } } .class private auto ansi ctest