From 133d6af5e5d819f4f87c4745c95fb3cc5d9094cb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 2 Apr 2023 18:53:14 +0200 Subject: [PATCH 1/6] Optimize range checks for arr[x % arr.Length] --- src/coreclr/jit/rangecheck.cpp | 18 ++++ src/tests/JIT/opt/RangeChecks/ModLength.cs | 98 +++++++++++++++++++ .../JIT/opt/RangeChecks/ModLength.csproj | 9 ++ 3 files changed, 125 insertions(+) create mode 100644 src/tests/JIT/opt/RangeChecks/ModLength.cs create mode 100644 src/tests/JIT/opt/RangeChecks/ModLength.csproj diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index 0462eab5ae842..cbc88a940a2b9 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -325,6 +325,24 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree* } } + if (m_pCompiler->vnStore->GetVNFunc(idxVn, &funcApp) && + ((funcApp.m_func == (VNFunc)GT_UMOD) || (funcApp.m_func == (VNFunc)GT_MOD))) + { + // We can always omit bound checks for Arr[X % Arr.Length] pattern. + // It should be MOD since both arguments are signed, but morph + // often converts "X MOD ARR_LEN" to "X UMOD ARR_LEN" so we handle both. + // + // if arr.Length is 0 we technically should keep the bounds check, but since the expression + // has to throw DividedByZeroException anyway - no special handling needed. + if (funcApp.m_args[1] == arrLenVn) + { + JITDUMP("[RangeCheck::OptimizeRangeCheck] [U]MOD(X, ARR_LEN) is always between bounds\n"); + m_pCompiler->optRemoveRangeCheck(bndsChk, comma, stmt); + m_updateStmt = true; + return; + } + } + // Get the range for this index. Range range = GetRange(block, treeIndex, false DEBUGARG(0)); diff --git a/src/tests/JIT/opt/RangeChecks/ModLength.cs b/src/tests/JIT/opt/RangeChecks/ModLength.cs new file mode 100644 index 0000000000000..43ee17ff6c787 --- /dev/null +++ b/src/tests/JIT/opt/RangeChecks/ModLength.cs @@ -0,0 +1,98 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; + +public class ModLength +{ + public static int Main() + { + Throws(() => Test1(new int[0], 0)); + Throws(() => Test2(new int[0], 1)); + Throws(() => Test3(new int[0], int.MaxValue)); + Throws(() => Test4(new int[0], 0)); + Throws(() => Test5(new int[0], 0)); + Throws(() => Test6(new int[0], 0)); + Throws(() => Test7(new int[0], 0)); + Throws(() => Test8(new int[0], 0)); + Throws(() => Test9(new int[0], 0)); + Test1(new int[1], 1); + Test2(new int[1], 1); + Throws(() => Test9(new int[1], 2)); + Test3(new int[1], int.MaxValue); + Throws(() => Test4(new int[1], int.MinValue)); + Test5(new int[1], -1); + Test6(new int[1], 1); + Test7(new int[1], 1); + Test8(new int[1], 1); + Test1(new int[10], 10); + Test2(new int[10], 11); + Test3(new int[10], int.MaxValue); + Throws(() => Test4(new int[10], int.MinValue)); + Throws(() => Test5(new int[10], -1)); + Test6(new int[10], 0); + Test7(new int[10], 0); + Throws(() => Test8(new int[10], 0)); + return 100; + } + + static void Throws(Action action, [CallerLineNumber] int line = 0) + { + try + { + action(); + } + catch (Exception e) + { + if (e is T) + { + return; + } + throw new InvalidOperationException($"{typeof(T)} exception was expected, actual: {e.GetType()}"); + } + throw new InvalidOperationException($"{typeof(T)} exception was expected"); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Test1(int[] arr, int index) => arr[index % arr.Length]; + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Test2(int[] arr, int index) => arr[(int)index % (int)arr.Length]; + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Test3(int[] arr, int index) => arr[(int)((uint)index % (uint)arr.Length)]; + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Test4(int[] arr, int index) => arr[arr.Length % index]; + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Test5(int[] arr, int index) + { + var span = arr.AsSpan(); + return arr[index % arr.Length]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Test6(int[] arr, int index) + { + var span = arr.AsSpan(); + return span[(int)index % (int)span.Length]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Test7(int[] arr, int index) + { + var span = arr.AsSpan(); + return span[(int)((uint)index % (uint)span.Length)]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Test8(int[] arr, int index) + { + var span = arr.AsSpan(); + return span[span.Length % index]; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Test9(int[] arr, int index) => arr[index / arr.Length]; +} diff --git a/src/tests/JIT/opt/RangeChecks/ModLength.csproj b/src/tests/JIT/opt/RangeChecks/ModLength.csproj new file mode 100644 index 0000000000000..6946bed81bfd5 --- /dev/null +++ b/src/tests/JIT/opt/RangeChecks/ModLength.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + From 214131c09f40e93c408926f97b852dbfa20ef58f Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 2 Apr 2023 19:38:24 +0200 Subject: [PATCH 2/6] Update ModLength.cs --- src/tests/JIT/opt/RangeChecks/ModLength.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tests/JIT/opt/RangeChecks/ModLength.cs b/src/tests/JIT/opt/RangeChecks/ModLength.cs index 43ee17ff6c787..765a99dd7d85d 100644 --- a/src/tests/JIT/opt/RangeChecks/ModLength.cs +++ b/src/tests/JIT/opt/RangeChecks/ModLength.cs @@ -1,6 +1,7 @@ // 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 class ModLength From 7a97a844b6b9167603f51a524d2d195a54f31ad0 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 2 Apr 2023 22:45:32 +0200 Subject: [PATCH 3/6] Update rangecheck.cpp --- src/coreclr/jit/rangecheck.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index cbc88a940a2b9..ebb8247ae6bd0 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -325,18 +325,15 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree* } } - if (m_pCompiler->vnStore->GetVNFunc(idxVn, &funcApp) && - ((funcApp.m_func == (VNFunc)GT_UMOD) || (funcApp.m_func == (VNFunc)GT_MOD))) + if (m_pCompiler->vnStore->GetVNFunc(idxVn, &funcApp) && (funcApp.m_func == (VNFunc)GT_UMOD)) { - // We can always omit bound checks for Arr[X % Arr.Length] pattern. - // It should be MOD since both arguments are signed, but morph - // often converts "X MOD ARR_LEN" to "X UMOD ARR_LEN" so we handle both. + // We can always omit bound checks for Arr[X u% Arr.Length] pattern. // // if arr.Length is 0 we technically should keep the bounds check, but since the expression // has to throw DividedByZeroException anyway - no special handling needed. if (funcApp.m_args[1] == arrLenVn) { - JITDUMP("[RangeCheck::OptimizeRangeCheck] [U]MOD(X, ARR_LEN) is always between bounds\n"); + JITDUMP("[RangeCheck::OptimizeRangeCheck] UMOD(X, ARR_LEN) is always between bounds\n"); m_pCompiler->optRemoveRangeCheck(bndsChk, comma, stmt); m_updateStmt = true; return; From 07458a1a83d128628b0c85e6e98beab96077cc4f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 2 Apr 2023 23:07:21 +0200 Subject: [PATCH 4/6] Improve ForCastOutput --- src/coreclr/jit/assertionprop.cpp | 14 +++++++++++--- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/rangecheck.cpp | 2 +- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index ebdf680fb10fe..b52686126275f 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -196,7 +196,7 @@ bool IntegralRange::Contains(int64_t value) const ForNode(node->AsQmark()->ElseNode(), compiler)); case GT_CAST: - return ForCastOutput(node->AsCast()); + return ForCastOutput(node->AsCast(), compiler); #if defined(FEATURE_HW_INTRINSICS) case GT_HWINTRINSIC: @@ -369,17 +369,25 @@ bool IntegralRange::Contains(int64_t value) const // Unlike ForCastInput, this method supports casts from floating point types. // // Arguments: -// cast - the cast node for which the range will be computed +// cast - the cast node for which the range will be computed +// compiler - Compiler object // // Return Value: // The range this cast produces - see description. // -/* static */ IntegralRange IntegralRange::ForCastOutput(GenTreeCast* cast) +/* static */ IntegralRange IntegralRange::ForCastOutput(GenTreeCast* cast, Compiler* compiler) { var_types fromType = genActualType(cast->CastOp()); var_types toType = cast->CastToType(); bool fromUnsigned = cast->IsUnsigned(); + // if we're upcasting and the cast op is a known non-negative - consider + // this cast unsigned + if (!fromUnsigned && (genTypeSize(toType) >= genTypeSize(fromType))) + { + fromUnsigned = cast->CastOp()->IsNeverNegative(compiler); + } + assert((fromType == TYP_INT) || (fromType == TYP_LONG) || varTypeIsFloating(fromType) || varTypeIsGC(fromType)); assert(varTypeIsIntegral(toType)); diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 67f24f096fc71..fdf8736ae0498 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -1347,7 +1347,7 @@ class IntegralRange static IntegralRange ForNode(GenTree* node, Compiler* compiler); static IntegralRange ForCastInput(GenTreeCast* cast); - static IntegralRange ForCastOutput(GenTreeCast* cast); + static IntegralRange ForCastOutput(GenTreeCast* cast, Compiler* compiler); static IntegralRange Union(IntegralRange range1, IntegralRange range2); #ifdef DEBUG diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index ebb8247ae6bd0..f9ad4c2c36500 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -327,7 +327,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree* if (m_pCompiler->vnStore->GetVNFunc(idxVn, &funcApp) && (funcApp.m_func == (VNFunc)GT_UMOD)) { - // We can always omit bound checks for Arr[X u% Arr.Length] pattern. + // We can always omit bound checks for Arr[X u% Arr.Length] pattern (unsigned MOD). // // if arr.Length is 0 we technically should keep the bounds check, but since the expression // has to throw DividedByZeroException anyway - no special handling needed. From d6f15f27344b208428f1f8388e5d96995b6b0f5d Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 2 Apr 2023 23:12:04 +0200 Subject: [PATCH 5/6] Fix assert --- src/coreclr/jit/assertionprop.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index b52686126275f..9abf1f83fac0f 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -381,13 +381,6 @@ bool IntegralRange::Contains(int64_t value) const var_types toType = cast->CastToType(); bool fromUnsigned = cast->IsUnsigned(); - // if we're upcasting and the cast op is a known non-negative - consider - // this cast unsigned - if (!fromUnsigned && (genTypeSize(toType) >= genTypeSize(fromType))) - { - fromUnsigned = cast->CastOp()->IsNeverNegative(compiler); - } - assert((fromType == TYP_INT) || (fromType == TYP_LONG) || varTypeIsFloating(fromType) || varTypeIsGC(fromType)); assert(varTypeIsIntegral(toType)); @@ -415,6 +408,13 @@ bool IntegralRange::Contains(int64_t value) const return ForCastInput(cast); } + // if we're upcasting and the cast op is a known non-negative - consider + // this cast unsigned + if (!fromUnsigned && (genTypeSize(toType) >= genTypeSize(fromType))) + { + fromUnsigned = cast->CastOp()->IsNeverNegative(compiler); + } + // CAST(uint/int <- ulong/long) - [INT_MIN..INT_MAX] // CAST(ulong/long <- uint) - [0..UINT_MAX] // CAST(ulong/long <- int) - [INT_MIN..INT_MAX] From 5d893b0402f8bf95efd8561b9543d49c41865178 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 2 Apr 2023 23:44:35 +0200 Subject: [PATCH 6/6] Changes in BCL --- .../src/System/Linq/Parallel/Utils/HashLookup.cs | 2 +- src/libraries/System.Linq/src/System/Linq/Lookup.cs | 2 +- .../src/System/Collections/Generic/Dictionary.cs | 2 +- .../System.Private.CoreLib/src/System/Threading/Timer.cs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Linq.Parallel/src/System/Linq/Parallel/Utils/HashLookup.cs b/src/libraries/System.Linq.Parallel/src/System/Linq/Parallel/Utils/HashLookup.cs index fae7beb9589b4..08ac4fa0995ab 100644 --- a/src/libraries/System.Linq.Parallel/src/System/Linq/Parallel/Utils/HashLookup.cs +++ b/src/libraries/System.Linq.Parallel/src/System/Linq/Parallel/Utils/HashLookup.cs @@ -78,7 +78,7 @@ private bool Find(TKey key, bool add, bool set, [MaybeNullWhen(false)] ref TValu { int hashCode = GetKeyHashCode(key); - for (int i = buckets[hashCode % buckets.Length] - 1; i >= 0; i = slots[i].next) + for (int i = buckets[(uint)hashCode % buckets.Length] - 1; i >= 0; i = slots[i].next) { if (slots[i].hashCode == hashCode && AreKeysEqual(slots[i].key, key)) { diff --git a/src/libraries/System.Linq/src/System/Linq/Lookup.cs b/src/libraries/System.Linq/src/System/Linq/Lookup.cs index 65ee9906894ac..2ee2d329255eb 100644 --- a/src/libraries/System.Linq/src/System/Linq/Lookup.cs +++ b/src/libraries/System.Linq/src/System/Linq/Lookup.cs @@ -196,7 +196,7 @@ private int InternalGetHashCode(TKey key) internal Grouping? GetGrouping(TKey key, bool create) { int hashCode = InternalGetHashCode(key); - for (Grouping? g = _groupings[hashCode % _groupings.Length]; g != null; g = g._hashNext) + for (Grouping? g = _groupings[(uint)hashCode % _groupings.Length]; g != null; g = g._hashNext) { if (g._hashCode == hashCode && _comparer.Equals(g._key, key)) { diff --git a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs index fe5f11e025198..75f509e24bae5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/Dictionary.cs @@ -1317,7 +1317,7 @@ private ref int GetBucket(uint hashCode) #if TARGET_64BIT return ref buckets[HashHelpers.FastMod(hashCode, (uint)buckets.Length, _fastModMultiplier)]; #else - return ref buckets[hashCode % (uint)buckets.Length]; + return ref buckets[(uint)hashCode % buckets.Length]; #endif } diff --git a/src/libraries/System.Private.CoreLib/src/System/Threading/Timer.cs b/src/libraries/System.Private.CoreLib/src/System/Threading/Timer.cs index 5cd51ae4086fa..6edff0f0eb414 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Threading/Timer.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Threading/Timer.cs @@ -508,7 +508,7 @@ internal TimerQueueTimer(TimerCallback timerCallback, object? state, uint dueTim { _executionContext = ExecutionContext.Capture(); } - _associatedTimerQueue = TimerQueue.Instances[Thread.GetCurrentProcessorId() % TimerQueue.Instances.Length]; + _associatedTimerQueue = TimerQueue.Instances[(uint)Thread.GetCurrentProcessorId() % TimerQueue.Instances.Length]; // After the following statement, the timer may fire. No more manipulation of timer state outside of // the lock is permitted beyond this point!