From 5c7fab27da8277b28c5b00f2354aa00edc15d280 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Mon, 5 Jul 2021 23:06:20 +0300 Subject: [PATCH 1/3] Add a test --- .../JitBlue/Runtime_54842/Runtime_54842.cs | 32 +++++++++++++++++++ .../Runtime_54842/Runtime_54842.csproj | 12 +++++++ 2 files changed, 44 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_54842/Runtime_54842.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_54842/Runtime_54842.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_54842/Runtime_54842.cs b/src/tests/JIT/Regression/JitBlue/Runtime_54842/Runtime_54842.cs new file mode 100644 index 0000000000000..8941e1c2d59e8 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_54842/Runtime_54842.cs @@ -0,0 +1,32 @@ +using System; +using System.Runtime.CompilerServices; + +public class Runtime_54842 +{ + public static int Main() + { + try + { + DoubleCheckedConvert(uint.MaxValue); + } + catch (OverflowException) + { + return 100; + } + + return 101; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static uint DoubleCheckedConvert(ulong a) + { + var b = (int)checked((uint)a); + + // Make sure the importer spills "b" to a local. + Use(b); + + return checked((uint)b); + } + + private static void Use(int value) { } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_54842/Runtime_54842.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_54842/Runtime_54842.csproj new file mode 100644 index 0000000000000..f3e1cbd44b404 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_54842/Runtime_54842.csproj @@ -0,0 +1,12 @@ + + + Exe + + + None + True + + + + + From 56c098319ef59adc7600c08ab81abfcec190db81 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Tue, 13 Jul 2021 21:40:00 +0300 Subject: [PATCH 2/3] Surgical fix for bad assertion generation Say we have a cast like this: CAST(uint <- long). What value does this tree compute? [int.MinValue..int.MaxValue] - IR operates on signed TYP_INTs. But assertion prop generated [0..uint.MaxValue] for it. The confusion created by this "how to interpret TYP_UINT" question caused a bug where for the assertion generated for the above cast, in the form of [0..uint.MaxValue], propagation could remove a checked cast in the form of CAST_OVF(uint < int). The proper fix is to generate proper ranges for such casts. The surgical fix proposed here is to always treat casts to TYP_UINT as if they were to TYP_INT. This is conservative, but always correct. The generated assertion is useless of course, but that makes this a zero-diff change. --- src/coreclr/jit/assertionprop.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index ffad32a61b200..7c511a17775d9 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -1271,6 +1271,10 @@ AssertionIndex Compiler::optCreateAssertion(GenTree* op1, } toType = op2->CastToType(); + if (toType == TYP_UINT) + { + toType = TYP_INT; + } SUBRANGE_COMMON: if ((assertionKind != OAK_SUBRANGE) && (assertionKind != OAK_EQUAL)) { From 3c04b13cc25310b532f1114a629a788ca708faad Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Wed, 14 Jul 2021 19:34:23 +0300 Subject: [PATCH 3/3] Add a comment explaining the quirk --- src/coreclr/jit/assertionprop.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 7c511a17775d9..ef18f2292ec5a 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -1271,6 +1271,15 @@ AssertionIndex Compiler::optCreateAssertion(GenTree* op1, } toType = op2->CastToType(); + + // Casts to TYP_UINT produce the same ranges as casts to TYP_INT, + // except in overflow cases which we do not yet handle. To avoid + // issues with the propagation code dropping, e. g., CAST_OVF(uint <- int) + // based on an assertion created from CAST(uint <- ulong), normalize the + // type for the range here. Note that TYP_ULONG theoretically has the same + // problem, but we do not create assertions for it. + // TODO-Cleanup: this assertion is not useful - this code exists to preserve + // previous behavior. Refactor it to stop generating such assertions. if (toType == TYP_UINT) { toType = TYP_INT;