Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Intrinsicify SpanHelpers.IndexOf(char) #22505

Merged
tannergooding merged 10 commits into from
Jun 13, 2019
Merged

Intrinsicify SpanHelpers.IndexOf(char) #22505

tannergooding merged 10 commits into from
Jun 13, 2019

Conversation

@benaadams
Copy link
Member

@benaadams benaadams commented Feb 10, 2019 •
edited
Loading

Based on #22875 which is first commit

and Vectorize wcslen

From #22187

 -------------------- |--------- |-----------:|
-      String_IndexOf |        7 |   5.031 ns |
+      String_IndexOf |        7 |   4.403 ns |
-      String_IndexOf |        8 |   5.053 ns |
+      String_IndexOf |        8 |   4.400 ns |
-      String_IndexOf |       15 |   8.817 ns |
+      String_IndexOf |       15 |   4.577 ns |
-      String_IndexOf |       16 |   8.223 ns |
+      String_IndexOf |       16 |   4.697 ns |
-      String_IndexOf |       31 |   9.292 ns |
+      String_IndexOf |       31 |   5.226 ns |
-      String_IndexOf |       32 |   9.276 ns |
+      String_IndexOf |       32 |   5.224 ns |
-      String_IndexOf |       63 |  11.307 ns |
+      String_IndexOf |       63 |   6.991 ns |
-      String_IndexOf |       64 |  11.196 ns |
+      String_IndexOf |       64 |   6.990 ns |
-      String_IndexOf |      127 |  14.386 ns |
+      String_IndexOf |      127 |   9.050 ns |
-      String_IndexOf |      128 |  14.643 ns |
+      String_IndexOf |      128 |   9.228 ns |
-      String_IndexOf |      255 |  21.395 ns |
+      String_IndexOf |      255 |  13.919 ns |
-      String_IndexOf |      256 |  21.452 ns |
+      String_IndexOf |      256 |  13.768 ns |
-      String_IndexOf |     1023 |  68.075 ns |
+      String_IndexOf |     1023 |  49.797 ns |
-      String_IndexOf |     1024 |  68.530 ns |
+      String_IndexOf |     1024 |  49.741 ns |

Will follow up after with the other methods as they depend on the helper methods at the bottom of this one.

/cc @CarolEidt @fiigii @tannergooding @jkotas

Resolves #22762

@benaadams benaadams mentioned this pull request Feb 11, 2019
Merged
@benaadams benaadams closed this Feb 15, 2019
@benaadams benaadams reopened this Feb 15, 2019
@benaadams
Copy link
Member Author

benaadams commented Feb 16, 2019 •
edited
Loading

Some kind of COM issue?

Windows_NT x64 Checked Build and Test (Jit - TieredCompilation=0 EnableIncompleteISAClass=1 FeatureSIMD=0)

MESSAGE:
cmdLine:D:\\j\\workspace\\x64_checked_w---8a49c5b2\\bin\\tests\\Windows_NT.x64.Checked\\Interop\\COM\\NativeClients\\Primitives\\Primitives.cmd Timed Out

Return code: -100
Raw output file: D:\\j\\workspace\\x64_checked_w---8a49c5b2\\bin\\tests\\Windows_NT.x64.Checked\\Reports\\Interop.COM\NativeClients\\Primitives\\Primitives.output.txt
Raw output:
BEGIN EXECUTION
Copying 'D:\\j\\workspace\\x64_checked_w---8a49c5b2\\bin\\tests\\Windows_NT.x64.Checked\\Tests\\Core_Root\\CoreShim.dll'...
 1 file(s) copied.
 \"D:\\j\\workspace\\x64_checked_w---8a49c5b2\\bin\\tests\\Windows_NT.x64.Checked\\Tests\\Core_Root\\corerun.exe\" Primitives.exe 
Searching for exe to launch in D:\\j\\workspace\\x64_checked_w---8a49c5b2\\bin\\tests\\Windows_NT.x64.Checked\\Interop\\COM\\NativeClients\\Primitives...
Launching 'D:\\j\\workspace\\x64_checked_w---8a49c5b2\\bin\\tests\\Windows_NT.x64.Checked\\Interop\\COM\\NativeClients\\Primitives\\COMClientPrimitives.exe'...

cmdLine:D:\\j\\workspace\\x64_checked_w---8a49c5b2\\bin\\tests\\Windows_NT.x64.Checked\\Interop\\COM\\NativeClients\\Primitives\\Primitives.cmd Timed Out
Test Harness Exitcode is : -100

To run the test:
> set CORE_ROOT=D:\\j\\workspace\\x64_checked_w---8a49c5b2\\bin\\tests\\Windows_NT.x64.Checked\\Tests\\Core_Root
> D:\\j\\workspace\\x64_checked_w---8a49c5b2\\bin\\tests\\Windows_NT.x64.Checked\\Interop\\COM\\NativeClients\\Primitives\\Primitives.cmd

Expected: True
Actual: False
+++++++++++++++++++
STACK TRACE:
at Interop_COM._NativeClients_Primitives_Primitives_._NativeClients_Primitives_Primitives_cmd() in D:\j\workspace\x64_checked_w---8a49c5b2\bin\tests\Windows_NT.x64.Checked\TestWrappers\Interop.COM\Interop.COM.XUnitWrapper.cs:line 179

@benaadams benaadams force-pushed the Intrinsicify-SpanHelpers.IndexOf(char) branch from e6b92b3 to dc6a029 Compare February 27, 2019 12:46
@benaadams
Copy link
Member Author

benaadams commented Feb 27, 2019 •
edited
Loading

Rebased for merge conflict

@benaadams benaadams force-pushed the Intrinsicify-SpanHelpers.IndexOf(char) branch from dc6a029 to 1d5b30d Compare February 27, 2019 12:56
@jkotas
Copy link
Member

jkotas commented Feb 27, 2019

@tannergooding @GrabYourPitchforks Could you please review this?

@benaadams benaadams mentioned this pull request Feb 27, 2019
Closed
tannergooding
tannergooding reviewed Feb 27, 2019
View reviewed changes
tannergooding
tannergooding reviewed Feb 27, 2019
View reviewed changes
tannergooding
tannergooding reviewed Feb 27, 2019
View reviewed changes
GrabYourPitchforks
GrabYourPitchforks reviewed Feb 28, 2019
View reviewed changes
@benaadams benaadams force-pushed the Intrinsicify-SpanHelpers.IndexOf(char) branch 2 times, most recently from af21a24 to b5db6e1 Compare April 6, 2019 04:14
@benaadams benaadams force-pushed the Intrinsicify-SpanHelpers.IndexOf(char) branch from b5db6e1 to 09aecea Compare April 6, 2019 04:23
@benaadams
Copy link
Member Author

benaadams commented Apr 6, 2019

Broke something 😢

@benaadams benaadams closed this Apr 6, 2019
@benaadams benaadams changed the title Intrinsicify SpanHelpers.IndexOf(char) [WIP] Intrinsicify SpanHelpers.IndexOf(char) May 28, 2019
@benaadams
Copy link
Member Author

benaadams commented May 28, 2019 •
edited
Loading

Still an issue on the alignment crossing pages; not seeing it though. Still looking.

@GrabYourPitchforks you'll be happy to know your test System.Tests.StringTests.Ctor_CharPtr_DoesNotAccessInvalidPage is picking it up

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Jun 4, 2019 •
edited
Loading

Still looking, but I believe the error is in this line in SpanHelpers.IndexOf:

if ((((nint)Unsafe.AsPointer(ref searchSpace) + (nint)offset) & (nint)(Vector256<ushort>.Count - 1)) != 0)

This essentially treats searchSpace as a byte* (not a char*), adds offset bytes (not chars) to the pointer, then sees if the whole thing is aligned to a 16-byte boundary. This means it's properly SSE2-aligned, but not AVX2-aligned. So a 256-bit read on data that's only guaranteed 128-bit aligned can indeed spill over to the next page and AV.

I believe this line should instead read:

if (((nint)Unsafe.AsPointer(ref Unsafe.Add(ref searchSpace, (IntPtr)offset)) & (nint)(Vector256<byte>.Count - 1)) != 0)

That performs char* (not a byte*) pointer arithmetic on the searchSpace reference, then checks for a 32-byte boundary, not a 16-byte boundary. The data should then be properly AVX2-aligned.

@benaadams
Copy link
Member Author

benaadams commented Jun 4, 2019

Still looking, but I believe the error is in this line ...

Ah-ah! Could also cast the return of AsPointer to (char*) prior to the add?

@benaadams benaadams changed the title [WIP] Intrinsicify SpanHelpers.IndexOf(char) Intrinsicify SpanHelpers.IndexOf(char) Jun 4, 2019
@benaadams
Copy link
Member Author

benaadams commented Jun 4, 2019

Btw, nice test :)

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Jun 4, 2019

Thanks - glad the test proved fruitful! :)

benaadams
benaadams commented Jun 4, 2019
View reviewed changes
@benaadams
Copy link
Member Author

benaadams commented Jun 13, 2019

Anyone fancy merging? Then I can update the other PRs that depend on the methods in this PR

@tannergooding
Copy link
Member

tannergooding commented Jun 13, 2019

Thanks for the work @benaadams, and sorry for the delay in getting this merged.

@tannergooding tannergooding merged commit 5676801 into dotnet:master Jun 13, 2019
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Jun 13, 2019
@benaadams @dotnet-bot
Intrinsicify SpanHelpers.IndexOf(char) (dotnet/coreclr#22505)
647f027 
* Helpers to support Intrinsics in SpanHelpers.Char

* Intrinsicify SpanHelpers.IndexOf(char)

* Feedback

* fix

* Fix assert

* Improve comment warning

* fix

* fix

* Fix

* Fix

Signed-off-by: dotnet-bot <[email protected]>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corert that referenced this pull request Jun 13, 2019
@benaadams @dotnet-bot
Intrinsicify SpanHelpers.IndexOf(char) (dotnet/coreclr#22505)
e7ef98b 
* Helpers to support Intrinsics in SpanHelpers.Char

* Intrinsicify SpanHelpers.IndexOf(char)

* Feedback

* fix

* Fix assert

* Improve comment warning

* fix

* fix

* Fix

* Fix

Signed-off-by: dotnet-bot <[email protected]>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/mono that referenced this pull request Jun 13, 2019
@benaadams @dotnet-bot
Intrinsicify SpanHelpers.IndexOf(char) (dotnet/coreclr#22505)
852ffe5 
* Helpers to support Intrinsics in SpanHelpers.Char

* Intrinsicify SpanHelpers.IndexOf(char)

* Feedback

* fix

* Fix assert

* Improve comment warning

* fix

* fix

* Fix

* Fix

Signed-off-by: dotnet-bot <[email protected]>
stephentoub pushed a commit to dotnet/corefx that referenced this pull request Jun 13, 2019
@benaadams @stephentoub
Intrinsicify SpanHelpers.IndexOf(char) (dotnet/coreclr#22505)
7d8c686 
* Helpers to support Intrinsics in SpanHelpers.Char

* Intrinsicify SpanHelpers.IndexOf(char)

* Feedback

* fix

* Fix assert

* Improve comment warning

* fix

* fix

* Fix

* Fix

Signed-off-by: dotnet-bot <[email protected]>
jkotas pushed a commit to dotnet/corert that referenced this pull request Jun 13, 2019
@benaadams @jkotas
Intrinsicify SpanHelpers.IndexOf(char) (dotnet/coreclr#22505)
f95e009 
* Helpers to support Intrinsics in SpanHelpers.Char

* Intrinsicify SpanHelpers.IndexOf(char)

* Feedback

* fix

* Fix assert

* Improve comment warning

* fix

* fix

* Fix

* Fix

Signed-off-by: dotnet-bot <[email protected]>
marek-safar pushed a commit to mono/mono that referenced this pull request Jun 14, 2019
@benaadams @marek-safar
Intrinsicify SpanHelpers.IndexOf(char) (dotnet/coreclr#22505)
12b737d 
* Helpers to support Intrinsics in SpanHelpers.Char

* Intrinsicify SpanHelpers.IndexOf(char)

* Feedback

* fix

* Fix assert

* Improve comment warning

* fix

* fix

* Fix

* Fix

Signed-off-by: dotnet-bot <[email protected]>
@benaadams benaadams deleted the Intrinsicify-SpanHelpers.IndexOf(char) branch November 9, 2019 14:49
This was referenced Jan 31, 2020
Closed
Closed
Closed
Closed
Closed
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@benaadams @tannergooding
Intrinsicify SpanHelpers.IndexOf(char) (dotnet/coreclr#22505)
641e08f 
* Helpers to support Intrinsics in SpanHelpers.Char

* Intrinsicify SpanHelpers.IndexOf(char)

* Feedback

* fix

* Fix assert

* Improve comment warning

* fix

* fix

* Fix

* Fix


Commit migrated from dotnet/coreclr@5676801
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

5 more reviewers

@pentp pentp pentp left review comments

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks left review comments

@jkotas jkotas jkotas left review comments

@mikedn mikedn mikedn left review comments

@tannergooding tannergooding tannergooding approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Memory

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use Intrinsics.X86 for SpanHelpers.IndexOf(char)

8 participants

@benaadams @jkotas @stephentoub @tannergooding @GrabYourPitchforks @pentp @mikedn @ahsonkhan