From 549d7c40ce4e2f3179199a78b09648c7b9fabebc 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 d18d4c729a8f8..86fba1692e8d6 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -995,11 +995,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); @@ -1007,6 +1011,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 7dfe8556be5c2a8c1267a4f02bc02238bc718339 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 86fba1692e8d6..d63eece7775cb 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -967,13 +967,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); @@ -987,13 +985,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; @@ -1001,16 +1000,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 @@ -1019,21 +1016,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 a44254944ace54bc92e816d79c5020eb099c8088 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 e46e469a08672..019269b318b31 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -999,16 +999,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 30889ac59fdb737d6d88a7c4bf159e32e32eea61 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 fbddd31bb3e0a..715ac704703c0 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 8627f6f4140af5bb08fd32a5e7abb3830b77a8a6 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 715ac704703c0..e09df9a97812c 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 ed83650b7d20e41be4c3cc35ad51684153cb83dc 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 e09df9a97812c..33f93aa4b7049 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 5d92816266eae2c048bf95c45da7707e29acae3d 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 33f93aa4b7049..bb5e651126ed5 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 9ac73fbdcb548..7b6381b6af231 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 0ee90f42863a5aa5bb5619e2c7c32d43675b5d18 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 0a7ca74ac13a5edb38ab8b7bd85509f815c784af 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 bb5e651126ed5..da0d840140d4a 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 7b6381b6af231..f9aeb83d0ed4b 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 c65192aaba8b91232591bc1f6e24b52148e0b6dd 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 da0d840140d4a..56c41c6103170 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 35679cb585c0447f86710778c0fad8fa1efca06c 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 56c41c6103170..90631e360f6b3 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);