From 8d9ae27fb5983c854164f6cc1f19743d86bbd2fc Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 19 Sep 2022 13:01:57 -0700 Subject: [PATCH 1/3] Fixing SpanHelpers.LastIndexOfAnyValueType to no longer create out of bounds GC refs --- .../src/System/SpanHelpers.T.cs | 211 ++++++++---------- 1 file changed, 99 insertions(+), 112 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs index 877c21311876d..eaec5c8c6d1a3 100644 --- a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs +++ b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs @@ -2153,59 +2153,57 @@ private static int LastIndexOfValueType(ref TValue searchSpace else if (Vector256.IsHardwareAccelerated && length >= Vector256.Count) { Vector256 equals, values = Vector256.Create(value); - ref TValue currentSearchSpace = ref Unsafe.Add(ref searchSpace, length - Vector256.Count); + nint offset = length - Vector256.Count; - // Loop until either we've finished all elements or there's less than a vector's-worth remaining. - do + // Loop until either we've finished all elements -or- there's one or less than a vector's-worth remaining. + while (offset > 0) { - equals = TNegator.NegateIfNeeded(Vector256.Equals(values, Vector256.LoadUnsafe(ref currentSearchSpace))); + equals = TNegator.NegateIfNeeded(Vector256.Equals(values, Vector256.LoadUnsafe(ref searchSpace, (nuint)(offset)))); + if (equals == Vector256.Zero) { - currentSearchSpace = ref Unsafe.Subtract(ref currentSearchSpace, Vector256.Count); + offset -= Vector256.Count; continue; } - return ComputeLastIndex(ref searchSpace, ref currentSearchSpace, equals); + return ComputeLastIndex(offset, equals); } - while (!Unsafe.IsAddressLessThan(ref currentSearchSpace, ref searchSpace)); // If any elements remain, process the first vector in the search space. - if ((uint)length % Vector256.Count != 0) + equals = TNegator.NegateIfNeeded(Vector256.Equals(values, Vector256.LoadUnsafe(ref searchSpace))); + + if (equals != Vector256.Zero) { - equals = TNegator.NegateIfNeeded(Vector256.Equals(values, Vector256.LoadUnsafe(ref searchSpace))); - if (equals != Vector256.Zero) - { - return ComputeLastIndex(ref searchSpace, ref searchSpace, equals); - } + return ComputeLastIndex(offset: 0, equals); } } else { Vector128 equals, values = Vector128.Create(value); - ref TValue currentSearchSpace = ref Unsafe.Add(ref searchSpace, length - Vector128.Count); + nint offset = length - Vector128.Count; - // Loop until either we've finished all elements or there's less than a vector's-worth remaining. - do + // Loop until either we've finished all elements -or- there's one or less than a vector's-worth remaining. + while (offset > 0) { - equals = TNegator.NegateIfNeeded(Vector128.Equals(values, Vector128.LoadUnsafe(ref currentSearchSpace))); + equals = TNegator.NegateIfNeeded(Vector128.Equals(values, Vector128.LoadUnsafe(ref searchSpace, (nuint)(offset)))); + if (equals == Vector128.Zero) { - currentSearchSpace = ref Unsafe.Subtract(ref currentSearchSpace, Vector128.Count); + offset -= Vector128.Count; continue; } - return ComputeLastIndex(ref searchSpace, ref currentSearchSpace, equals); + return ComputeLastIndex(offset, equals); } - while (!Unsafe.IsAddressLessThan(ref currentSearchSpace, ref searchSpace)); + // If any elements remain, process the first vector in the search space. - if ((uint)length % Vector128.Count != 0) + + equals = TNegator.NegateIfNeeded(Vector128.Equals(values, Vector128.LoadUnsafe(ref searchSpace))); + + if (equals != Vector128.Zero) { - equals = TNegator.NegateIfNeeded(Vector128.Equals(values, Vector128.LoadUnsafe(ref searchSpace))); - if (equals != Vector128.Zero) - { - return ComputeLastIndex(ref searchSpace, ref searchSpace, equals); - } + return ComputeLastIndex(offset: 0, equals); } } @@ -2291,63 +2289,60 @@ private static int LastIndexOfAnyValueType(ref TValue searchSp else if (Vector256.IsHardwareAccelerated && length >= Vector256.Count) { Vector256 equals, current, values0 = Vector256.Create(value0), values1 = Vector256.Create(value1); - ref TValue currentSearchSpace = ref Unsafe.Add(ref searchSpace, length - Vector256.Count); + nint offset = length - Vector256.Count; // Loop until either we've finished all elements or there's less than a vector's-worth remaining. - do + while (offset > 0) { - current = Vector256.LoadUnsafe(ref currentSearchSpace); + current = Vector256.LoadUnsafe(ref searchSpace, (nuint)(offset)); equals = TNegator.NegateIfNeeded(Vector256.Equals(current, values0) | Vector256.Equals(current, values1)); + if (equals == Vector256.Zero) { - currentSearchSpace = ref Unsafe.Subtract(ref currentSearchSpace, Vector256.Count); + offset -= Vector256.Count; continue; } - return ComputeLastIndex(ref searchSpace, ref currentSearchSpace, equals); + return ComputeLastIndex(offset, equals); } - while (!Unsafe.IsAddressLessThan(ref currentSearchSpace, ref searchSpace)); // If any elements remain, process the first vector in the search space. - if ((uint)length % Vector256.Count != 0) + + current = Vector256.LoadUnsafe(ref searchSpace); + equals = TNegator.NegateIfNeeded(Vector256.Equals(current, values0) | Vector256.Equals(current, values1)); + + if (equals != Vector256.Zero) { - current = Vector256.LoadUnsafe(ref searchSpace); - equals = TNegator.NegateIfNeeded(Vector256.Equals(current, values0) | Vector256.Equals(current, values1)); - if (equals != Vector256.Zero) - { - return ComputeLastIndex(ref searchSpace, ref searchSpace, equals); - } + return ComputeLastIndex(offset: 0, equals); } } else { Vector128 equals, current, values0 = Vector128.Create(value0), values1 = Vector128.Create(value1); - ref TValue currentSearchSpace = ref Unsafe.Add(ref searchSpace, length - Vector128.Count); + nint offset = length - Vector128.Count; // Loop until either we've finished all elements or there's less than a vector's-worth remaining. - do + while (offset > 0) { - current = Vector128.LoadUnsafe(ref currentSearchSpace); + current = Vector128.LoadUnsafe(ref searchSpace, (nuint)(offset)); equals = TNegator.NegateIfNeeded(Vector128.Equals(current, values0) | Vector128.Equals(current, values1)); if (equals == Vector128.Zero) { - currentSearchSpace = ref Unsafe.Subtract(ref currentSearchSpace, Vector128.Count); + offset -= Vector128.Count; continue; } - return ComputeLastIndex(ref searchSpace, ref currentSearchSpace, equals); + return ComputeLastIndex(offset, equals); } - while (!Unsafe.IsAddressLessThan(ref currentSearchSpace, ref searchSpace)); // If any elements remain, process the first vector in the search space. - if ((uint)length % Vector128.Count != 0) + + current = Vector128.LoadUnsafe(ref searchSpace); + equals = TNegator.NegateIfNeeded(Vector128.Equals(current, values0) | Vector128.Equals(current, values1)); + + if (equals != Vector128.Zero) { - current = Vector128.LoadUnsafe(ref searchSpace); - equals = TNegator.NegateIfNeeded(Vector128.Equals(current, values0) | Vector128.Equals(current, values1)); - if (equals != Vector128.Zero) - { - return ComputeLastIndex(ref searchSpace, ref searchSpace, equals); - } + return ComputeLastIndex(offset: 0, equals); } } @@ -2433,63 +2428,61 @@ private static int LastIndexOfAnyValueType(ref TValue searchSp else if (Vector256.IsHardwareAccelerated && length >= Vector256.Count) { Vector256 equals, current, values0 = Vector256.Create(value0), values1 = Vector256.Create(value1), values2 = Vector256.Create(value2); - ref TValue currentSearchSpace = ref Unsafe.Add(ref searchSpace, length - Vector256.Count); + nint offset = length - Vector256.Count; // Loop until either we've finished all elements or there's less than a vector's-worth remaining. - do + while (offset > 0) { - current = Vector256.LoadUnsafe(ref currentSearchSpace); + current = Vector256.LoadUnsafe(ref searchSpace, (nuint)(offset)); equals = TNegator.NegateIfNeeded(Vector256.Equals(current, values0) | Vector256.Equals(current, values1) | Vector256.Equals(current, values2)); + if (equals == Vector256.Zero) { - currentSearchSpace = ref Unsafe.Subtract(ref currentSearchSpace, Vector256.Count); + offset -= Vector256.Count; continue; } - return ComputeLastIndex(ref searchSpace, ref currentSearchSpace, equals); + return ComputeLastIndex(offset, equals); } - while (!Unsafe.IsAddressLessThan(ref currentSearchSpace, ref searchSpace)); // If any elements remain, process the first vector in the search space. - if ((uint)length % Vector256.Count != 0) + + current = Vector256.LoadUnsafe(ref searchSpace); + equals = TNegator.NegateIfNeeded(Vector256.Equals(current, values0) | Vector256.Equals(current, values1) | Vector256.Equals(current, values2)); + + if (equals != Vector256.Zero) { - current = Vector256.LoadUnsafe(ref searchSpace); - equals = TNegator.NegateIfNeeded(Vector256.Equals(current, values0) | Vector256.Equals(current, values1) | Vector256.Equals(current, values2)); - if (equals != Vector256.Zero) - { - return ComputeLastIndex(ref searchSpace, ref searchSpace, equals); - } + return ComputeLastIndex(offset: 0, equals); } } else { Vector128 equals, current, values0 = Vector128.Create(value0), values1 = Vector128.Create(value1), values2 = Vector128.Create(value2); - ref TValue currentSearchSpace = ref Unsafe.Add(ref searchSpace, length - Vector128.Count); + nint offset = length - Vector128.Count; // Loop until either we've finished all elements or there's less than a vector's-worth remaining. - do + while (offset > 0) { - current = Vector128.LoadUnsafe(ref currentSearchSpace); + current = Vector128.LoadUnsafe(ref searchSpace, (nuint)(offset)); equals = TNegator.NegateIfNeeded(Vector128.Equals(current, values0) | Vector128.Equals(current, values1) | Vector128.Equals(current, values2)); + if (equals == Vector128.Zero) { - currentSearchSpace = ref Unsafe.Subtract(ref currentSearchSpace, Vector128.Count); + offset -= Vector128.Count; continue; } - return ComputeLastIndex(ref searchSpace, ref currentSearchSpace, equals); + return ComputeLastIndex(offset, equals); } - while (!Unsafe.IsAddressLessThan(ref currentSearchSpace, ref searchSpace)); // If any elements remain, process the first vector in the search space. - if ((uint)length % Vector128.Count != 0) + + current = Vector128.LoadUnsafe(ref searchSpace); + equals = TNegator.NegateIfNeeded(Vector128.Equals(current, values0) | Vector128.Equals(current, values1) | Vector128.Equals(current, values2)); + + if (equals != Vector128.Zero) { - current = Vector128.LoadUnsafe(ref searchSpace); - equals = TNegator.NegateIfNeeded(Vector128.Equals(current, values0) | Vector128.Equals(current, values1) | Vector128.Equals(current, values2)); - if (equals != Vector128.Zero) - { - return ComputeLastIndex(ref searchSpace, ref searchSpace, equals); - } + return ComputeLastIndex(offset: 0, equals); } } @@ -2547,67 +2540,61 @@ private static int LastIndexOfAnyValueType(ref TValue searchSp else if (Vector256.IsHardwareAccelerated && length >= Vector256.Count) { Vector256 equals, current, values0 = Vector256.Create(value0), values1 = Vector256.Create(value1), values2 = Vector256.Create(value2), values3 = Vector256.Create(value3); - ref TValue currentSearchSpace = ref Unsafe.Add(ref searchSpace, length - Vector256.Count); + nint offset = length - Vector256.Count; // Loop until either we've finished all elements or there's less than a vector's-worth remaining. - do + while (offset > 0) { - current = Vector256.LoadUnsafe(ref currentSearchSpace); + current = Vector256.LoadUnsafe(ref searchSpace, (nuint)(offset)); equals = TNegator.NegateIfNeeded(Vector256.Equals(current, values0) | Vector256.Equals(current, values1) | Vector256.Equals(current, values2) | Vector256.Equals(current, values3)); if (equals == Vector256.Zero) { - currentSearchSpace = ref Unsafe.Subtract(ref currentSearchSpace, Vector256.Count); + offset -= Vector256.Count; continue; } - return ComputeLastIndex(ref searchSpace, ref currentSearchSpace, equals); + return ComputeLastIndex(offset, equals); } - while (!Unsafe.IsAddressLessThan(ref currentSearchSpace, ref searchSpace)); // If any elements remain, process the first vector in the search space. - if ((uint)length % Vector256.Count != 0) + + current = Vector256.LoadUnsafe(ref searchSpace); + equals = TNegator.NegateIfNeeded(Vector256.Equals(current, values0) | Vector256.Equals(current, values1) | Vector256.Equals(current, values2) | Vector256.Equals(current, values3)); + + if (equals != Vector256.Zero) { - current = Vector256.LoadUnsafe(ref searchSpace); - equals = TNegator.NegateIfNeeded(Vector256.Equals(current, values0) | Vector256.Equals(current, values1) - | Vector256.Equals(current, values2) | Vector256.Equals(current, values3)); - if (equals != Vector256.Zero) - { - return ComputeLastIndex(ref searchSpace, ref searchSpace, equals); - } + return ComputeLastIndex(offset: 0, equals); } } else { Vector128 equals, current, values0 = Vector128.Create(value0), values1 = Vector128.Create(value1), values2 = Vector128.Create(value2), values3 = Vector128.Create(value3); - ref TValue currentSearchSpace = ref Unsafe.Add(ref searchSpace, length - Vector128.Count); + nint offset = length - Vector128.Count; // Loop until either we've finished all elements or there's less than a vector's-worth remaining. - do + while (offset > 0) { - current = Vector128.LoadUnsafe(ref currentSearchSpace); - equals = TNegator.NegateIfNeeded(Vector128.Equals(current, values0) | Vector128.Equals(current, values1) - | Vector128.Equals(current, values2) | Vector128.Equals(current, values3)); + current = Vector128.LoadUnsafe(ref searchSpace, (nuint)(offset)); + equals = TNegator.NegateIfNeeded(Vector128.Equals(current, values0) | Vector128.Equals(current, values1) | Vector128.Equals(current, values2) | Vector128.Equals(current, values3)); + if (equals == Vector128.Zero) { - currentSearchSpace = ref Unsafe.Subtract(ref currentSearchSpace, Vector128.Count); + offset -= Vector128.Count; continue; } - return ComputeLastIndex(ref searchSpace, ref currentSearchSpace, equals); + return ComputeLastIndex(offset, equals); } - while (!Unsafe.IsAddressLessThan(ref currentSearchSpace, ref searchSpace)); // If any elements remain, process the first vector in the search space. - if ((uint)length % Vector128.Count != 0) + + current = Vector128.LoadUnsafe(ref searchSpace); + equals = TNegator.NegateIfNeeded(Vector128.Equals(current, values0) | Vector128.Equals(current, values1) | Vector128.Equals(current, values2) | Vector128.Equals(current, values3)); + + if (equals != Vector128.Zero) { - current = Vector128.LoadUnsafe(ref searchSpace); - equals = TNegator.NegateIfNeeded(Vector128.Equals(current, values0) | Vector128.Equals(current, values1) - | Vector128.Equals(current, values2) | Vector128.Equals(current, values3)); - if (equals != Vector128.Zero) - { - return ComputeLastIndex(ref searchSpace, ref searchSpace, equals); - } + return ComputeLastIndex(offset: 0, equals); } } @@ -2631,19 +2618,19 @@ private static int ComputeFirstIndex(ref T searchSpace, ref T current, Vector } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static int ComputeLastIndex(ref T searchSpace, ref T current, Vector128 equals) where T : struct + private static int ComputeLastIndex(nint offset, Vector128 equals) where T : struct { uint notEqualsElements = equals.ExtractMostSignificantBits(); int index = 31 - BitOperations.LeadingZeroCount(notEqualsElements); // 31 = 32 (bits in Int32) - 1 (indexing from zero) - return (int)(Unsafe.ByteOffset(ref searchSpace, ref current) / Unsafe.SizeOf()) + index; + return (int)(offset / Unsafe.SizeOf()) + index; } [MethodImpl(MethodImplOptions.AggressiveInlining)] - private static int ComputeLastIndex(ref T searchSpace, ref T current, Vector256 equals) where T : struct + private static int ComputeLastIndex(nint offset, Vector256 equals) where T : struct { uint notEqualsElements = equals.ExtractMostSignificantBits(); int index = 31 - BitOperations.LeadingZeroCount(notEqualsElements); // 31 = 32 (bits in Int32) - 1 (indexing from zero) - return (int)(Unsafe.ByteOffset(ref searchSpace, ref current) / Unsafe.SizeOf()) + index; + return (int)(offset / Unsafe.SizeOf()) + index; } private interface INegator where T : struct From 35e563c162105235a376418119a8e90a4e6e8877 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 19 Sep 2022 13:19:33 -0700 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Adam Sitnik --- .../System.Private.CoreLib/src/System/SpanHelpers.T.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs index eaec5c8c6d1a3..2d8d5adf788f1 100644 --- a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs +++ b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs @@ -2622,7 +2622,7 @@ private static int ComputeLastIndex(nint offset, Vector128 equals) where T { uint notEqualsElements = equals.ExtractMostSignificantBits(); int index = 31 - BitOperations.LeadingZeroCount(notEqualsElements); // 31 = 32 (bits in Int32) - 1 (indexing from zero) - return (int)(offset / Unsafe.SizeOf()) + index; + return (int)offset + index; } [MethodImpl(MethodImplOptions.AggressiveInlining)] @@ -2630,7 +2630,7 @@ private static int ComputeLastIndex(nint offset, Vector256 equals) where T { uint notEqualsElements = equals.ExtractMostSignificantBits(); int index = 31 - BitOperations.LeadingZeroCount(notEqualsElements); // 31 = 32 (bits in Int32) - 1 (indexing from zero) - return (int)(offset / Unsafe.SizeOf()) + index; + return (int)offset + index; } private interface INegator where T : struct From cd0d155b992021e6b182e1f00f6782fd3bc7727a Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Mon, 19 Sep 2022 13:25:37 -0700 Subject: [PATCH 3/3] Adjusting the comment as per PR review feedback --- .../src/System/SpanHelpers.T.cs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs index 2d8d5adf788f1..2cb5c28fef430 100644 --- a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs +++ b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs @@ -2169,7 +2169,8 @@ private static int LastIndexOfValueType(ref TValue searchSpace return ComputeLastIndex(offset, equals); } - // If any elements remain, process the first vector in the search space. + // Process the first vector in the search space. + equals = TNegator.NegateIfNeeded(Vector256.Equals(values, Vector256.LoadUnsafe(ref searchSpace))); if (equals != Vector256.Zero) @@ -2197,7 +2198,7 @@ private static int LastIndexOfValueType(ref TValue searchSpace } - // If any elements remain, process the first vector in the search space. + // Process the first vector in the search space. equals = TNegator.NegateIfNeeded(Vector128.Equals(values, Vector128.LoadUnsafe(ref searchSpace))); @@ -2306,7 +2307,7 @@ private static int LastIndexOfAnyValueType(ref TValue searchSp return ComputeLastIndex(offset, equals); } - // If any elements remain, process the first vector in the search space. + // Process the first vector in the search space. current = Vector256.LoadUnsafe(ref searchSpace); equals = TNegator.NegateIfNeeded(Vector256.Equals(current, values0) | Vector256.Equals(current, values1)); @@ -2335,7 +2336,7 @@ private static int LastIndexOfAnyValueType(ref TValue searchSp return ComputeLastIndex(offset, equals); } - // If any elements remain, process the first vector in the search space. + // Process the first vector in the search space. current = Vector128.LoadUnsafe(ref searchSpace); equals = TNegator.NegateIfNeeded(Vector128.Equals(current, values0) | Vector128.Equals(current, values1)); @@ -2445,7 +2446,7 @@ private static int LastIndexOfAnyValueType(ref TValue searchSp return ComputeLastIndex(offset, equals); } - // If any elements remain, process the first vector in the search space. + // Process the first vector in the search space. current = Vector256.LoadUnsafe(ref searchSpace); equals = TNegator.NegateIfNeeded(Vector256.Equals(current, values0) | Vector256.Equals(current, values1) | Vector256.Equals(current, values2)); @@ -2475,7 +2476,7 @@ private static int LastIndexOfAnyValueType(ref TValue searchSp return ComputeLastIndex(offset, equals); } - // If any elements remain, process the first vector in the search space. + // Process the first vector in the search space. current = Vector128.LoadUnsafe(ref searchSpace); equals = TNegator.NegateIfNeeded(Vector128.Equals(current, values0) | Vector128.Equals(current, values1) | Vector128.Equals(current, values2)); @@ -2557,7 +2558,7 @@ private static int LastIndexOfAnyValueType(ref TValue searchSp return ComputeLastIndex(offset, equals); } - // If any elements remain, process the first vector in the search space. + // Process the first vector in the search space. current = Vector256.LoadUnsafe(ref searchSpace); equals = TNegator.NegateIfNeeded(Vector256.Equals(current, values0) | Vector256.Equals(current, values1) | Vector256.Equals(current, values2) | Vector256.Equals(current, values3)); @@ -2587,7 +2588,7 @@ private static int LastIndexOfAnyValueType(ref TValue searchSp return ComputeLastIndex(offset, equals); } - // If any elements remain, process the first vector in the search space. + // Process the first vector in the search space. current = Vector128.LoadUnsafe(ref searchSpace); equals = TNegator.NegateIfNeeded(Vector128.Equals(current, values0) | Vector128.Equals(current, values1) | Vector128.Equals(current, values2) | Vector128.Equals(current, values3));