From 090f739a5dc2727a2268a71b82320aa603178179 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Tue, 22 Mar 2022 22:35:31 +0100 Subject: [PATCH 01/11] Optimized string.Replace(char, char) vector code path --- .../src/System/String.Manipulation.cs | 24 ++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index e9df30a7d78ec..ca8622d759910 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1014,11 +1014,15 @@ public string Replace(char oldChar, char newChar) Vector oldChars = new Vector(oldChar); Vector newChars = new Vector(newChar); + Vector original; + Vector equals; + Vector results; + do { - Vector original = Unsafe.ReadUnaligned>(ref Unsafe.As(ref pSrc)); - Vector equals = Vector.Equals(original, oldChars); - Vector results = Vector.ConditionalSelect(equals, newChars, original); + original = Unsafe.ReadUnaligned>(ref Unsafe.As(ref pSrc)); + equals = Vector.Equals(original, oldChars); + results = Vector.ConditionalSelect(equals, newChars, original); Unsafe.WriteUnaligned(ref Unsafe.As(ref pDst), results); pSrc = ref Unsafe.Add(ref pSrc, Vector.Count); @@ -1026,6 +1030,20 @@ public string Replace(char oldChar, char newChar) remainingLength -= Vector.Count; } while (remainingLength >= Vector.Count); + + // There are [0, Vector.Count) elements remaining now. + // As the operation is idempotent, and we know that in total there are at least Vector.Count + // elements available, we read a vector from the very end of the string, perform the replace + // and write to the destination at the very end. + // Thus we can eliminate the scalar processing of the remaining elements. + // We perform this operation even if there are 0 elements remaining, as it is cheaper than the + // additional check which would introduce a branch here. + original = Unsafe.ReadUnaligned>(ref Unsafe.As(ref Unsafe.Add(ref _firstChar, Length - Vector.Count))); + equals = Vector.Equals(original, oldChars); + results = Vector.ConditionalSelect(equals, newChars, original); + Unsafe.WriteUnaligned(ref Unsafe.As(ref Unsafe.Add(ref result._firstChar, result.Length - Vector.Count)), results); + + return result; } for (; remainingLength > 0; remainingLength--) From d366d9336252bf6d4ba7a04331223899e207b89c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Wed, 23 Mar 2022 15:46:17 +0100 Subject: [PATCH 02/11] Optimized code pathes even further --- .../src/System/String.Manipulation.cs | 50 +++++++++---------- 1 file changed, 23 insertions(+), 27 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index ca8622d759910..1b74f794ce10d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -986,13 +986,11 @@ private string ReplaceCore(string oldValue, string? newValue, CompareInfo? ci, C // public string Replace(char oldChar, char newChar) { - if (oldChar == newChar) - return this; - - int firstIndex = IndexOf(oldChar); - - if (firstIndex < 0) + int firstIndex; + if (oldChar == newChar || (firstIndex = IndexOf(oldChar)) < 0) + { return this; + } int remainingLength = Length - firstIndex; string result = FastAllocateString(Length); @@ -1006,13 +1004,14 @@ public string Replace(char oldChar, char newChar) } // Copy the remaining characters, doing the replacement as we go. - ref ushort pSrc = ref Unsafe.Add(ref Unsafe.As(ref _firstChar), copyLength); - ref ushort pDst = ref Unsafe.Add(ref Unsafe.As(ref result._firstChar), copyLength); + ref ushort pSrc = ref Unsafe.Add(ref Unsafe.As(ref _firstChar), (nint)(uint)copyLength); + ref ushort pDst = ref Unsafe.Add(ref Unsafe.As(ref result._firstChar), (nint)(uint)copyLength); + nint i = 0; if (Vector.IsHardwareAccelerated && remainingLength >= Vector.Count) { - Vector oldChars = new Vector(oldChar); - Vector newChars = new Vector(newChar); + Vector oldChars = new(oldChar); + Vector newChars = new(newChar); Vector original; Vector equals; @@ -1020,16 +1019,14 @@ public string Replace(char oldChar, char newChar) do { - original = Unsafe.ReadUnaligned>(ref Unsafe.As(ref pSrc)); + original = Unsafe.ReadUnaligned>(ref Unsafe.As(ref Unsafe.Add(ref pSrc, i))); equals = Vector.Equals(original, oldChars); results = Vector.ConditionalSelect(equals, newChars, original); - Unsafe.WriteUnaligned(ref Unsafe.As(ref pDst), results); + Unsafe.WriteUnaligned(ref Unsafe.As(ref Unsafe.Add(ref pDst, i)), results); - pSrc = ref Unsafe.Add(ref pSrc, Vector.Count); - pDst = ref Unsafe.Add(ref pDst, Vector.Count); - remainingLength -= Vector.Count; + i += Vector.Count; } - while (remainingLength >= Vector.Count); + while (i <= (nint)(uint)(remainingLength - Vector.Count)); // There are [0, Vector.Count) elements remaining now. // As the operation is idempotent, and we know that in total there are at least Vector.Count @@ -1038,21 +1035,20 @@ public string Replace(char oldChar, char newChar) // Thus we can eliminate the scalar processing of the remaining elements. // We perform this operation even if there are 0 elements remaining, as it is cheaper than the // additional check which would introduce a branch here. - original = Unsafe.ReadUnaligned>(ref Unsafe.As(ref Unsafe.Add(ref _firstChar, Length - Vector.Count))); + + i = (nint)(uint)this.Length - Vector.Count; + original = Unsafe.ReadUnaligned>(ref Unsafe.As(ref Unsafe.Add(ref _firstChar, i))); equals = Vector.Equals(original, oldChars); results = Vector.ConditionalSelect(equals, newChars, original); - Unsafe.WriteUnaligned(ref Unsafe.As(ref Unsafe.Add(ref result._firstChar, result.Length - Vector.Count)), results); - - return result; + Unsafe.WriteUnaligned(ref Unsafe.As(ref Unsafe.Add(ref result._firstChar, i)), results); } - - for (; remainingLength > 0; remainingLength--) + else { - ushort currentChar = pSrc; - pDst = currentChar == oldChar ? newChar : currentChar; - - pSrc = ref Unsafe.Add(ref pSrc, 1); - pDst = ref Unsafe.Add(ref pDst, 1); + for (; i < (nint)(uint)remainingLength; ++i) + { + ushort currentChar = Unsafe.Add(ref pSrc, i); + Unsafe.Add(ref pDst, i) = currentChar == oldChar ? newChar : currentChar; + } } return result; From b4dcd15461212e7883a20ce6c452a578657b29c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Wed, 13 Apr 2022 16:32:08 +0200 Subject: [PATCH 03/11] Do vectorized operation at the end of the string only once When the remaining length is a multiple of the vector size, then the remainder is processed twice. This is redundant, and not needed. This commit changes that, so that the remainder is processed only once when the remaining elements match. --- .../src/System/String.Manipulation.cs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 1b74f794ce10d..1f73093302f0e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1017,16 +1017,21 @@ public string Replace(char oldChar, char newChar) Vector equals; Vector results; - do + nint lengthToExamine = (nint)(uint)(remainingLength - Vector.Count); + + if (lengthToExamine > 0) { - original = Unsafe.ReadUnaligned>(ref Unsafe.As(ref Unsafe.Add(ref pSrc, i))); - equals = Vector.Equals(original, oldChars); - results = Vector.ConditionalSelect(equals, newChars, original); - Unsafe.WriteUnaligned(ref Unsafe.As(ref Unsafe.Add(ref pDst, i)), results); + do + { + original = Unsafe.ReadUnaligned>(ref Unsafe.As(ref Unsafe.Add(ref pSrc, i))); + equals = Vector.Equals(original, oldChars); + results = Vector.ConditionalSelect(equals, newChars, original); + Unsafe.WriteUnaligned(ref Unsafe.As(ref Unsafe.Add(ref pDst, i)), results); - i += Vector.Count; + i += Vector.Count; + } + while (i < lengthToExamine); } - while (i <= (nint)(uint)(remainingLength - Vector.Count)); // There are [0, Vector.Count) elements remaining now. // As the operation is idempotent, and we know that in total there are at least Vector.Count From 424f16bdb3fc3ce0e49adb25ee6b3dfe45883c33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Wed, 20 Jul 2022 20:53:02 +0200 Subject: [PATCH 04/11] Don't use trick for collapsed epilogs Cf. https://github.com/dotnet/runtime/pull/67049#discussion_r833689895 --- .../src/System/String.Manipulation.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 1f73093302f0e..6e6bd40b1d48b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -986,11 +986,13 @@ private string ReplaceCore(string oldValue, string? newValue, CompareInfo? ci, C // public string Replace(char oldChar, char newChar) { - int firstIndex; - if (oldChar == newChar || (firstIndex = IndexOf(oldChar)) < 0) - { + if (oldChar == newChar) + return this; + + int firstIndex = IndexOf(oldChar); + + if (firstIndex < 0) return this; - } int remainingLength = Length - firstIndex; string result = FastAllocateString(Length); From 98c3f6a18ddfab5c5f90ac1e7818d2d558815a2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Wed, 20 Jul 2022 21:14:16 +0200 Subject: [PATCH 05/11] Handle remainder vectorized even if remainingLength <= Vector.Count and added tests for this --- src/libraries/Common/tests/Tests/System/StringTests.cs | 8 ++++++-- .../src/System/String.Manipulation.cs | 10 +++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/libraries/Common/tests/Tests/System/StringTests.cs b/src/libraries/Common/tests/Tests/System/StringTests.cs index 7d2558a959324..a6463fe3192aa 100644 --- a/src/libraries/Common/tests/Tests/System/StringTests.cs +++ b/src/libraries/Common/tests/Tests/System/StringTests.cs @@ -4697,14 +4697,18 @@ public static void Remove_Invalid() [InlineData("Aaaaaaaa", 'A', 'a', "aaaaaaaa")] // Single iteration of vectorised path; no remainders through non-vectorised path // Three leading 'a's before a match (copyLength > 0), Single iteration of vectorised path; no remainders through non-vectorised path [InlineData("aaaAaaaaaaa", 'A', 'a', "aaaaaaaaaaa")] - // Single iteration of vectorised path; 3 remainders through non-vectorised path + // Single iteration of vectorised path; 3 remainders handled by vectorized path [InlineData("AaaaaaaaaAa", 'A', 'a', "aaaaaaaaaaa")] + // Eight chars before a match (copyLength > 0), single iteration of vectorized path for the remainder + [InlineData("12345678AAAAAAA", 'A', 'a', "12345678aaaaaaaa")] // ------------------------- For Vector.Count == 16 (AVX2) ------------------------- [InlineData("AaaaaaaaAaaaaaaa", 'A', 'a', "aaaaaaaaaaaaaaaa")] // Single iteration of vectorised path; no remainders through non-vectorised path // Three leading 'a's before a match (copyLength > 0), Single iteration of vectorised path; no remainders through non-vectorised path [InlineData("aaaAaaaaaaaAaaaaaaa", 'A', 'a', "aaaaaaaaaaaaaaaaaaa")] - // Single iteration of vectorised path; 3 remainders through non-vectorised path + // Single iteration of vectorised path; 3 remainders handled by vectorized path [InlineData("AaaaaaaaAaaaaaaaaAa", 'A', 'a', "aaaaaaaaaaaaaaaaaaa")] + // Sixteen chars before a match (copyLength > 0), single iteration of vectorized path for the remainder + [InlineData("1234567890123456AAAAAAAAAAAAAAAA", 'A', 'a', "1234567890123456aaaaaaaaaaaaaaa")] // ----------------------------------- General test data ----------------------------------- [InlineData("Hello", 'l', '!', "He!!o")] // 2 match, non-vectorised path [InlineData("Hello", 'e', 'e', "Hello")] // oldChar and newChar are same; nothing to replace diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 6e6bd40b1d48b..8b9d6ff494322 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -994,7 +994,7 @@ public string Replace(char oldChar, char newChar) if (firstIndex < 0) return this; - int remainingLength = Length - firstIndex; + nint remainingLength = (nint)(uint)(Length - firstIndex); string result = FastAllocateString(Length); int copyLength = firstIndex; @@ -1010,7 +1010,7 @@ public string Replace(char oldChar, char newChar) ref ushort pDst = ref Unsafe.Add(ref Unsafe.As(ref result._firstChar), (nint)(uint)copyLength); nint i = 0; - if (Vector.IsHardwareAccelerated && remainingLength >= Vector.Count) + if (Vector.IsHardwareAccelerated && Length >= Vector.Count) { Vector oldChars = new(oldChar); Vector newChars = new(newChar); @@ -1019,7 +1019,7 @@ public string Replace(char oldChar, char newChar) Vector equals; Vector results; - nint lengthToExamine = (nint)(uint)(remainingLength - Vector.Count); + nint lengthToExamine = remainingLength - Vector.Count; if (lengthToExamine > 0) { @@ -1043,7 +1043,7 @@ public string Replace(char oldChar, char newChar) // We perform this operation even if there are 0 elements remaining, as it is cheaper than the // additional check which would introduce a branch here. - i = (nint)(uint)this.Length - Vector.Count; + i = (nint)(uint)Length - Vector.Count; original = Unsafe.ReadUnaligned>(ref Unsafe.As(ref Unsafe.Add(ref _firstChar, i))); equals = Vector.Equals(original, oldChars); results = Vector.ConditionalSelect(equals, newChars, original); @@ -1051,7 +1051,7 @@ public string Replace(char oldChar, char newChar) } else { - for (; i < (nint)(uint)remainingLength; ++i) + for (; i < remainingLength; ++i) { ushort currentChar = Unsafe.Add(ref pSrc, i); Unsafe.Add(ref pDst, i) = currentChar == oldChar ? newChar : currentChar; From 0a395c069ce0045b3688f3765b2c35a522505ef5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Wed, 20 Jul 2022 21:26:52 +0200 Subject: [PATCH 06/11] Introduce (internal) Vector.LoadUnsafe and Vector.StoreUnsafe and use it in string.Replace(char, char) --- .../src/System/Numerics/Vector.cs | 16 ++++++++++++++++ .../src/System/String.Manipulation.cs | 8 ++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs b/src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs index b9eb29598bb6e..d7b7ec55511fb 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs @@ -895,6 +895,14 @@ public static bool LessThanOrEqualAll(Vector left, Vector right) public static bool LessThanOrEqualAny(Vector left, Vector right) where T : struct => LessThanOrEqual(left, right).As() != Vector.Zero; + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal static Vector LoadUnsafe(ref T source, nint elementOffset) + where T : struct + { + source = ref Unsafe.Add(ref source, elementOffset); + return Unsafe.ReadUnaligned>(ref Unsafe.As(ref source)); + } + /// Computes the maximum of two vectors on a per-element basis. /// The vector to compare with . /// The vector to compare with . @@ -1658,6 +1666,14 @@ public static Vector SquareRoot(Vector value) return result; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal static void StoreUnsafe(this Vector source, ref T destination, nint elementOffset) + where T : struct + { + destination = ref Unsafe.Add(ref destination, elementOffset); + Unsafe.WriteUnaligned(ref Unsafe.As(ref destination), source); + } + /// Subtracts two vectors to compute their difference. /// The vector from which will be subtracted. /// The vector to subtract from . diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 8b9d6ff494322..6855977ab9e47 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1025,10 +1025,10 @@ public string Replace(char oldChar, char newChar) { do { - original = Unsafe.ReadUnaligned>(ref Unsafe.As(ref Unsafe.Add(ref pSrc, i))); + original = Vector.LoadUnsafe(ref pSrc, i); equals = Vector.Equals(original, oldChars); results = Vector.ConditionalSelect(equals, newChars, original); - Unsafe.WriteUnaligned(ref Unsafe.As(ref Unsafe.Add(ref pDst, i)), results); + results.StoreUnsafe(ref pDst, i); i += Vector.Count; } @@ -1044,10 +1044,10 @@ public string Replace(char oldChar, char newChar) // additional check which would introduce a branch here. i = (nint)(uint)Length - Vector.Count; - original = Unsafe.ReadUnaligned>(ref Unsafe.As(ref Unsafe.Add(ref _firstChar, i))); + original = Vector.LoadUnsafe(ref Unsafe.As(ref _firstChar), i); equals = Vector.Equals(original, oldChars); results = Vector.ConditionalSelect(equals, newChars, original); - Unsafe.WriteUnaligned(ref Unsafe.As(ref Unsafe.Add(ref result._firstChar, i)), results); + results.StoreUnsafe(ref Unsafe.As(ref result._firstChar), i); } else { From c5f47404a6c3d1f8d66d845fed412c2ebd71b742 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Wed, 20 Jul 2022 21:36:15 +0200 Subject: [PATCH 07/11] Avoid Unsafe.As reinterpret casts by introducing string.GetRawStringDataAsUshort() internal method --- .../src/System/String.Manipulation.cs | 8 ++++---- src/libraries/System.Private.CoreLib/src/System/String.cs | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 6855977ab9e47..19e83b3d70f90 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1006,8 +1006,8 @@ public string Replace(char oldChar, char newChar) } // Copy the remaining characters, doing the replacement as we go. - ref ushort pSrc = ref Unsafe.Add(ref Unsafe.As(ref _firstChar), (nint)(uint)copyLength); - ref ushort pDst = ref Unsafe.Add(ref Unsafe.As(ref result._firstChar), (nint)(uint)copyLength); + ref ushort pSrc = ref Unsafe.Add(ref GetRawStringDataAsUshort(), (nint)(uint)copyLength); + ref ushort pDst = ref Unsafe.Add(ref result.GetRawStringDataAsUshort(), (nint)(uint)copyLength); nint i = 0; if (Vector.IsHardwareAccelerated && Length >= Vector.Count) @@ -1044,10 +1044,10 @@ public string Replace(char oldChar, char newChar) // additional check which would introduce a branch here. i = (nint)(uint)Length - Vector.Count; - original = Vector.LoadUnsafe(ref Unsafe.As(ref _firstChar), i); + original = Vector.LoadUnsafe(ref GetRawStringDataAsUshort(), i); equals = Vector.Equals(original, oldChars); results = Vector.ConditionalSelect(equals, newChars, original); - results.StoreUnsafe(ref Unsafe.As(ref result._firstChar), i); + results.StoreUnsafe(ref result.GetRawStringDataAsUshort(), i); } else { diff --git a/src/libraries/System.Private.CoreLib/src/System/String.cs b/src/libraries/System.Private.CoreLib/src/System/String.cs index 77d2168b0d38b..30251de5cce5f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.cs @@ -508,6 +508,7 @@ public static bool IsNullOrWhiteSpace([NotNullWhen(false)] string? value) public ref readonly char GetPinnableReference() => ref _firstChar; internal ref char GetRawStringData() => ref _firstChar; + internal ref ushort GetRawStringDataAsUshort() => ref Unsafe.As(ref _firstChar); // Helper for encodings so they can talk to our buffer directly // stringLength must be the exact size we'll expect From 568622513292a09ca90fc9ca769a8157f4f18d3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Wed, 20 Jul 2022 22:59:01 +0200 Subject: [PATCH 08/11] Fixed copy/paste error (from local dev to repo) --- src/libraries/Common/tests/Tests/System/StringTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Common/tests/Tests/System/StringTests.cs b/src/libraries/Common/tests/Tests/System/StringTests.cs index a6463fe3192aa..5d2ec4e79a89a 100644 --- a/src/libraries/Common/tests/Tests/System/StringTests.cs +++ b/src/libraries/Common/tests/Tests/System/StringTests.cs @@ -4700,7 +4700,7 @@ public static void Remove_Invalid() // Single iteration of vectorised path; 3 remainders handled by vectorized path [InlineData("AaaaaaaaaAa", 'A', 'a', "aaaaaaaaaaa")] // Eight chars before a match (copyLength > 0), single iteration of vectorized path for the remainder - [InlineData("12345678AAAAAAA", 'A', 'a', "12345678aaaaaaaa")] + [InlineData("12345678AAAAAAA", 'A', 'a', "12345678aaaaaaa")] // ------------------------- For Vector.Count == 16 (AVX2) ------------------------- [InlineData("AaaaaaaaAaaaaaaa", 'A', 'a', "aaaaaaaaaaaaaaaa")] // Single iteration of vectorised path; no remainders through non-vectorised path // Three leading 'a's before a match (copyLength > 0), Single iteration of vectorised path; no remainders through non-vectorised path @@ -4708,7 +4708,7 @@ public static void Remove_Invalid() // Single iteration of vectorised path; 3 remainders handled by vectorized path [InlineData("AaaaaaaaAaaaaaaaaAa", 'A', 'a', "aaaaaaaaaaaaaaaaaaa")] // Sixteen chars before a match (copyLength > 0), single iteration of vectorized path for the remainder - [InlineData("1234567890123456AAAAAAAAAAAAAAAA", 'A', 'a', "1234567890123456aaaaaaaaaaaaaaa")] + [InlineData("1234567890123456AAAAAAAAAAAAAAA", 'A', 'a', "1234567890123456aaaaaaaaaaaaaaa")] // ----------------------------------- General test data ----------------------------------- [InlineData("Hello", 'l', '!', "He!!o")] // 2 match, non-vectorised path [InlineData("Hello", 'e', 'e', "Hello")] // oldChar and newChar are same; nothing to replace From d875e1f263fee7821274b65451b2ea8f0a38a545 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Mon, 15 Aug 2022 11:35:21 +0200 Subject: [PATCH 09/11] PR Feedback --- .../src/System/Numerics/Vector.cs | 4 ++-- .../src/System/String.Manipulation.cs | 18 +++++++++--------- .../src/System/String.cs | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs b/src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs index d7b7ec55511fb..5dee06aa6adf0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Numerics/Vector.cs @@ -896,7 +896,7 @@ public static bool LessThanOrEqualAny(Vector left, Vector right) where T : struct => LessThanOrEqual(left, right).As() != Vector.Zero; [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal static Vector LoadUnsafe(ref T source, nint elementOffset) + internal static Vector LoadUnsafe(ref T source, nuint elementOffset) where T : struct { source = ref Unsafe.Add(ref source, elementOffset); @@ -1667,7 +1667,7 @@ public static Vector SquareRoot(Vector value) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal static void StoreUnsafe(this Vector source, ref T destination, nint elementOffset) + internal static void StoreUnsafe(this Vector source, ref T destination, nuint elementOffset) where T : struct { destination = ref Unsafe.Add(ref destination, elementOffset); diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 19e83b3d70f90..0fc8fb8ab516c 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -994,7 +994,7 @@ public string Replace(char oldChar, char newChar) if (firstIndex < 0) return this; - nint remainingLength = (nint)(uint)(Length - firstIndex); + nuint remainingLength = (uint)(Length - firstIndex); string result = FastAllocateString(Length); int copyLength = firstIndex; @@ -1006,9 +1006,9 @@ public string Replace(char oldChar, char newChar) } // Copy the remaining characters, doing the replacement as we go. - ref ushort pSrc = ref Unsafe.Add(ref GetRawStringDataAsUshort(), (nint)(uint)copyLength); - ref ushort pDst = ref Unsafe.Add(ref result.GetRawStringDataAsUshort(), (nint)(uint)copyLength); - nint i = 0; + ref ushort pSrc = ref Unsafe.Add(ref GetRawStringDataAsUInt16(), (uint)copyLength); + ref ushort pDst = ref Unsafe.Add(ref result.GetRawStringDataAsUInt16(), (uint)copyLength); + nuint i = 0; if (Vector.IsHardwareAccelerated && Length >= Vector.Count) { @@ -1019,7 +1019,7 @@ public string Replace(char oldChar, char newChar) Vector equals; Vector results; - nint lengthToExamine = remainingLength - Vector.Count; + nuint lengthToExamine = remainingLength - (nuint)Vector.Count; if (lengthToExamine > 0) { @@ -1030,7 +1030,7 @@ public string Replace(char oldChar, char newChar) results = Vector.ConditionalSelect(equals, newChars, original); results.StoreUnsafe(ref pDst, i); - i += Vector.Count; + i += (nuint)Vector.Count; } while (i < lengthToExamine); } @@ -1043,11 +1043,11 @@ public string Replace(char oldChar, char newChar) // We perform this operation even if there are 0 elements remaining, as it is cheaper than the // additional check which would introduce a branch here. - i = (nint)(uint)Length - Vector.Count; - original = Vector.LoadUnsafe(ref GetRawStringDataAsUshort(), i); + i = (nuint)((uint)Length - Vector.Count); + original = Vector.LoadUnsafe(ref GetRawStringDataAsUInt16(), i); equals = Vector.Equals(original, oldChars); results = Vector.ConditionalSelect(equals, newChars, original); - results.StoreUnsafe(ref result.GetRawStringDataAsUshort(), i); + results.StoreUnsafe(ref result.GetRawStringDataAsUInt16(), i); } else { diff --git a/src/libraries/System.Private.CoreLib/src/System/String.cs b/src/libraries/System.Private.CoreLib/src/System/String.cs index 30251de5cce5f..6fdbdfdc449e5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.cs @@ -508,7 +508,7 @@ public static bool IsNullOrWhiteSpace([NotNullWhen(false)] string? value) public ref readonly char GetPinnableReference() => ref _firstChar; internal ref char GetRawStringData() => ref _firstChar; - internal ref ushort GetRawStringDataAsUshort() => ref Unsafe.As(ref _firstChar); + internal ref ushort GetRawStringDataAsUInt16() => ref Unsafe.As(ref _firstChar); // Helper for encodings so they can talk to our buffer directly // stringLength must be the exact size we'll expect From d011202ab08a764efb376172af3a1b4c468522fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Mon, 15 Aug 2022 14:04:31 +0200 Subject: [PATCH 10/11] Fixed bug and added tests for this --- src/libraries/Common/tests/Tests/System/StringTests.cs | 4 ++++ .../System.Private.CoreLib/src/System/String.Manipulation.cs | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libraries/Common/tests/Tests/System/StringTests.cs b/src/libraries/Common/tests/Tests/System/StringTests.cs index 5d2ec4e79a89a..c16a31a1f6014 100644 --- a/src/libraries/Common/tests/Tests/System/StringTests.cs +++ b/src/libraries/Common/tests/Tests/System/StringTests.cs @@ -4699,6 +4699,8 @@ public static void Remove_Invalid() [InlineData("aaaAaaaaaaa", 'A', 'a', "aaaaaaaaaaa")] // Single iteration of vectorised path; 3 remainders handled by vectorized path [InlineData("AaaaaaaaaAa", 'A', 'a', "aaaaaaaaaaa")] + // Single iteration of vectorized path; 0 remainders handled by vectorized path + [InlineData("aaaaaaaaaAa", 'A', 'a', "aaaaaaaaaaa")] // Eight chars before a match (copyLength > 0), single iteration of vectorized path for the remainder [InlineData("12345678AAAAAAA", 'A', 'a', "12345678aaaaaaa")] // ------------------------- For Vector.Count == 16 (AVX2) ------------------------- @@ -4707,6 +4709,8 @@ public static void Remove_Invalid() [InlineData("aaaAaaaaaaaAaaaaaaa", 'A', 'a', "aaaaaaaaaaaaaaaaaaa")] // Single iteration of vectorised path; 3 remainders handled by vectorized path [InlineData("AaaaaaaaAaaaaaaaaAa", 'A', 'a', "aaaaaaaaaaaaaaaaaaa")] + // Single iteration of vectorized path; 0 remainders handled by vectorized path + [InlineData("aaaaaaaaaaaaaaaaaAa", 'A', 'a', "aaaaaaaaaaaaaaaaaaa")] // Sixteen chars before a match (copyLength > 0), single iteration of vectorized path for the remainder [InlineData("1234567890123456AAAAAAAAAAAAAAA", 'A', 'a', "1234567890123456aaaaaaaaaaaaaaa")] // ----------------------------------- General test data ----------------------------------- diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 0fc8fb8ab516c..a984208591f9e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1021,7 +1021,7 @@ public string Replace(char oldChar, char newChar) nuint lengthToExamine = remainingLength - (nuint)Vector.Count; - if (lengthToExamine > 0) + if ((nint)lengthToExamine > 0) { do { @@ -1043,7 +1043,7 @@ public string Replace(char oldChar, char newChar) // We perform this operation even if there are 0 elements remaining, as it is cheaper than the // additional check which would introduce a branch here. - i = (nuint)((uint)Length - Vector.Count); + i = (uint)(Length - Vector.Count); original = Vector.LoadUnsafe(ref GetRawStringDataAsUInt16(), i); equals = Vector.Equals(original, oldChars); results = Vector.ConditionalSelect(equals, newChars, original); From 7b8adcea2e78c12df1d525b893453f1dc4ed7dba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Mon, 15 Aug 2022 20:01:45 +0200 Subject: [PATCH 11/11] Make condition about lengthToExamine clearer as suggested --- .../src/System/String.Manipulation.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index a984208591f9e..675a8e188477b 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1019,10 +1019,10 @@ public string Replace(char oldChar, char newChar) Vector equals; Vector results; - nuint lengthToExamine = remainingLength - (nuint)Vector.Count; - - if ((nint)lengthToExamine > 0) + if (remainingLength > (nuint)Vector.Count) { + nuint lengthToExamine = remainingLength - (nuint)Vector.Count; + do { original = Vector.LoadUnsafe(ref pSrc, i);