From 004b61a0a5c095a88dbe674aff25b22045a69564 Mon Sep 17 00:00:00 2001 From: TIHan Date: Wed, 3 May 2023 14:00:46 -0700 Subject: [PATCH 1/6] Do not remove CAST nodes on assignment if the LCL_VAR is a parameter. --- src/coreclr/jit/morph.cpp | 11 ++-- .../JitBlue/Runtime_84693/Runtime_84693.cs | 2 +- .../JitBlue/Runtime_85382/Runtime_85382.cs | 51 +++++++++++++++++++ .../Runtime_85382/Runtime_85382.csproj | 8 +++ 4 files changed, 68 insertions(+), 4 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_85382/Runtime_85382.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_85382/Runtime_85382.csproj diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 4cfbfb2eeb63e..42a2e3b339ac4 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -10189,9 +10189,14 @@ GenTree* Compiler::fgOptimizeCastOnAssignment(GenTreeOp* asg) if (!effectiveOp1->OperIs(GT_IND, GT_LCL_VAR, GT_LCL_FLD)) return asg; - if (effectiveOp1->OperIs(GT_LCL_VAR) && - !lvaGetDesc(effectiveOp1->AsLclVarCommon()->GetLclNum())->lvNormalizeOnLoad()) - return asg; + if (effectiveOp1->OperIs(GT_LCL_VAR)) + { + LclVarDsc* varDsc = lvaGetDesc(effectiveOp1->AsLclVarCommon()->GetLclNum()); + + // It is not safe to remove the cast for non-NormalizeOnLoad variables or parameters. + if (!varDsc->lvNormalizeOnLoad() || varDsc->lvIsParam) + return asg; + } if (op2->gtOverflow()) return asg; diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_84693/Runtime_84693.cs b/src/tests/JIT/Regression/JitBlue/Runtime_84693/Runtime_84693.cs index 19e39efcffa63..65e6202aabbe4 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_84693/Runtime_84693.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_84693/Runtime_84693.cs @@ -28,7 +28,7 @@ public static int M8(byte arg0) } } - [ActiveIssue("https://github.com/dotnet/runtime/issues/85081")] + [Fact] public static int TestEntryPoint() { var result = Test.Program.M8(1); if (result != 255) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_85382/Runtime_85382.cs b/src/tests/JIT/Regression/JitBlue/Runtime_85382/Runtime_85382.cs new file mode 100644 index 0000000000000..454793d554d82 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_85382/Runtime_85382.cs @@ -0,0 +1,51 @@ +// 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; +using System.Collections.Generic; +using System.Runtime.CompilerServices; +using Xunit; + +public class Test +{ + // This is trying to verify that we properly zero-extend on all platforms. + public class Program + { + public static long s_15; + public static sbyte s_17; + public static ushort s_21 = 36659; + public static int Test() + { + s_15 = ~1; + return M40(0); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void Consume(ushort x) { } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static int M40(ushort arg0) + { + for (int var0 = 0; var0 < 2; var0++) + { + arg0 = 65535; + arg0 &= (ushort)(s_15++ >> s_17); + arg0 %= s_21; + } + + Consume(arg0); + + if (arg0 != 28876) + { + return 0; + } + return 100; + } + } + + [Fact] + public static int TestEntryPoint() { + return Test.Program.Test(); + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_85382/Runtime_85382.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_85382/Runtime_85382.csproj new file mode 100644 index 0000000000000..15edd99711a1a --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_85382/Runtime_85382.csproj @@ -0,0 +1,8 @@ + + + True + + + + + \ No newline at end of file From 56570b7334c357b550f8249e2c7661e343d0e066 Mon Sep 17 00:00:00 2001 From: TIHan Date: Wed, 3 May 2023 17:01:17 -0700 Subject: [PATCH 2/6] Added NormalizeOnLoad rules from SingleAccretion. Added description of why we cannot remove CAST nodes from parameters. --- src/coreclr/jit/compiler.h | 8 ++++++++ src/coreclr/jit/morph.cpp | 2 ++ 2 files changed, 10 insertions(+) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 4420a7662a36c..97168fb030857 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -1106,6 +1106,14 @@ class LclVarDsc assert(lvIsStructField); return ((lvFldOffset % TARGET_POINTER_SIZE) == 0); } + + // NormalizeOnLoad Rules: + // 1. All small locals are actually TYP_INT locals. + // 2. NOL locals are such that not all definitions can be controlled by the compiler and so the upper bits can + // be undefined.For parameters this is the case because of ABI.For struct fields - because of padding.For + // address - exposed locals - because not all stores are direct. + // 3. Hence, all NOL uses(unless proven otherwise) are assumed in morph to have undefined upper bits and + // explicit casts have be inserted to "normalize" them back to conform to IL semantics. bool lvNormalizeOnLoad() const { return varTypeIsSmall(TypeGet()) && diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 42a2e3b339ac4..893f92444c329 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -10194,6 +10194,8 @@ GenTree* Compiler::fgOptimizeCastOnAssignment(GenTreeOp* asg) LclVarDsc* varDsc = lvaGetDesc(effectiveOp1->AsLclVarCommon()->GetLclNum()); // It is not safe to remove the cast for non-NormalizeOnLoad variables or parameters. + // The reason why the parameters are not safe is because there are issues surrounding + // NormalizeOnLoad variables in VN and/or CSE. if (!varDsc->lvNormalizeOnLoad() || varDsc->lvIsParam) return asg; } From e68875b110f4461b31391adad1a7f716980b92bc Mon Sep 17 00:00:00 2001 From: TIHan Date: Fri, 5 May 2023 13:25:33 -0700 Subject: [PATCH 3/6] Remove morph optimization for NormalizeOnLoad in fgMorphLocalVar. Update test. --- src/coreclr/jit/morph.cpp | 17 ------- .../JitBlue/Runtime_85382/Runtime_85382.cs | 49 ++++++++++++++++++- 2 files changed, 48 insertions(+), 18 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 893f92444c329..a76804f04f078 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -4898,23 +4898,6 @@ GenTree* Compiler::fgMorphLocalVar(GenTree* tree) var_types lclVarType = varDsc->TypeGet(); bool isBoolQuirk = lclVarType == TYP_BOOL; - // Assertion prop can tell us to omit adding a cast here. This is - // useful when the local is a small-typed parameter that is passed in a - // register: in that case, the ABI specifies that the upper bits might - // be invalid, but the assertion guarantees us that we have normalized - // when we wrote it. - if (optLocalAssertionProp && !isBoolQuirk && - optAssertionIsSubrange(tree, IntegralRange::ForType(lclVarType), apFull) != NO_ASSERTION_INDEX) - { - // The previous assertion can guarantee us that if this node gets - // assigned a register, it will be normalized already. It is still - // possible that this node ends up being in memory, in which case - // normalization will still be needed, so we better have the right - // type. - assert(tree->TypeGet() == varDsc->TypeGet()); - return tree; - } - // Small-typed arguments and aliased locals are normalized on load. // Other small-typed locals are normalized on store. // Also, under the debugger as the debugger could write to the variable. diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_85382/Runtime_85382.cs b/src/tests/JIT/Regression/JitBlue/Runtime_85382/Runtime_85382.cs index 454793d554d82..0b39426f7d436 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_85382/Runtime_85382.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_85382/Runtime_85382.cs @@ -44,8 +44,55 @@ public static int M40(ushort arg0) } } + public class Program2 + { + public static long s_15; + public static sbyte s_17; + public static ushort s_21 = 36659; + public static int Test() + { + s_15 = ~1; + return M40(); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void Consume(ushort x) { } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static int M40() + { + S s = default; + for (int var0 = 0; var0 < 2; var0++) + { + s.U = 65535; + s.U &= (ushort)(s_15++ >> s_17); + s.U %= s_21; + } + + Consume(s.U); + + if (s.U != 28876) + { + return 0; + } + return 100; + } + + public struct S { public ushort U; } + } + [Fact] public static int TestEntryPoint() { - return Test.Program.Test(); + if (Test.Program.Test() != 100) + { + return 0; + } + + if (Test.Program2.Test() != 100) + { + return 0; + } + + return 100; } } From b1116270b060aa72efb63d448f5654b2cb290c3f Mon Sep 17 00:00:00 2001 From: TIHan Date: Mon, 17 Jul 2023 16:06:48 -0700 Subject: [PATCH 4/6] Do not OptimizeCastOnStore for params and struct fields --- src/coreclr/jit/morph.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 3babf06e54161..bdf52b26065c9 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -10115,8 +10115,14 @@ GenTree* Compiler::fgOptimizeCastOnStore(GenTree* store) if (!src->OperIs(GT_CAST)) return store; - if (store->OperIs(GT_STORE_LCL_VAR) && !lvaGetDesc(store->AsLclVarCommon())->lvNormalizeOnLoad()) - return store; + if (store->OperIs(GT_STORE_LCL_VAR)) + { + LclVarDsc* varDsc = lvaGetDesc(store->AsLclVarCommon()->GetLclNum()); + + // It is not safe to remove the cast for non-NormalizeOnLoad variables, parameters or struct fields. + if (!varDsc->lvNormalizeOnLoad() || varDsc->lvIsParam || varDsc->lvIsStructField) + return store; + } if (src->gtOverflow()) return store; From d6417d50bfda2bf0d983c80094dd5abb2e738ebe Mon Sep 17 00:00:00 2001 From: Will Smith Date: Tue, 18 Jul 2023 13:49:24 -0700 Subject: [PATCH 5/6] Update src/coreclr/jit/morph.cpp Co-authored-by: Jakob Botsch Nielsen --- src/coreclr/jit/morph.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index bdf52b26065c9..875c68b6664c8 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -10119,8 +10119,11 @@ GenTree* Compiler::fgOptimizeCastOnStore(GenTree* store) { LclVarDsc* varDsc = lvaGetDesc(store->AsLclVarCommon()->GetLclNum()); - // It is not safe to remove the cast for non-NormalizeOnLoad variables, parameters or struct fields. - if (!varDsc->lvNormalizeOnLoad() || varDsc->lvIsParam || varDsc->lvIsStructField) + // We can make this transformation only under the assumption that NOL locals are always normalized before they are used, + // however this is not always the case: the JIT will utilize subrange assertions for NOL locals to make normalization + // assumptions -- see fgMorphLeafLocal. Thus we can only do this for cases where we know for sure that subsequent uses + // will normalize, which we can only guarantee when the local is address exposed. + if (!varDsc->lvNormalizeOnLoad() || !varDsc->IsAddressExposed()) return store; } From 0b728de7a3cef04882498fa334ce35a1aea5f4b1 Mon Sep 17 00:00:00 2001 From: TIHan Date: Wed, 19 Jul 2023 09:43:35 -0700 Subject: [PATCH 6/6] Formatting --- src/coreclr/jit/morph.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 875c68b6664c8..7564722729092 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -10119,9 +10119,12 @@ GenTree* Compiler::fgOptimizeCastOnStore(GenTree* store) { LclVarDsc* varDsc = lvaGetDesc(store->AsLclVarCommon()->GetLclNum()); - // We can make this transformation only under the assumption that NOL locals are always normalized before they are used, - // however this is not always the case: the JIT will utilize subrange assertions for NOL locals to make normalization - // assumptions -- see fgMorphLeafLocal. Thus we can only do this for cases where we know for sure that subsequent uses + // We can make this transformation only under the assumption that NOL locals are always normalized before they + // are used, + // however this is not always the case: the JIT will utilize subrange assertions for NOL locals to make + // normalization + // assumptions -- see fgMorphLeafLocal. Thus we can only do this for cases where we know for sure that + // subsequent uses // will normalize, which we can only guarantee when the local is address exposed. if (!varDsc->lvNormalizeOnLoad() || !varDsc->IsAddressExposed()) return store;