Intrinsics support for WidenFourAsciiBytesToUtf16AndWriteToBuffer by pgovind · Pull Request #38597 · dotnet/runtime

pgovind · 2020-06-30T02:17:36Z

Implements 1 API from #35034

ghost · 2020-06-30T02:17:39Z

Tagging subscribers to this area: @tannergooding
Notify danmosemsft if you want to be subscribed.

GrabYourPitchforks · 2020-07-01T04:48:14Z

src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs

Does the use of ToScalar here produce acceptable codegen? In the SSE2 code paths, analyzing the x86 codegen revealed that ConvertToUInt64 produced better codegen than ToScalar. I don't know if the arm64 JIT has a similar behavior.

I believe we have fixed most of these cases and improved the codegen for things like ToScalar. If not, we should as that is preferable for readability.

Yes, ToScalar() is optimized for arm64.

GrabYourPitchforks · 2020-07-01T04:49:25Z

Control flow LGTM, but I don't know enough about advsimd intrinsics to give this a proper review. @tannergooding, thoughts?
I really need to sit down with an arm64 machine for a few days and tinker. :)

tannergooding · 2020-07-01T16:53:26Z

src/libraries/System.Utf8String.Experimental/src/System/Runtime/Intrinsics/Intrinsics.Shims.cs

@GrabYourPitchforks, instead of maintaining this Shims.cs file, we could just include the *.PlatformNotSupported.cs variants of the relevant ISAs: https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/AdvSimd.PlatformNotSupported.cs

Thoughts? (Note we still need the Shims file for Vector64/Vector128/Vector256, but it would avoid needing to update for all of the ISAs)

I'm curious about this. Whenever you select "Go to definition" in Visual Studio, it takes you to the AdvSimd.PlatformNotSupported.cs file. Why do we have the same APIs throwing PNSE in two different files?

Edit: Santi explained the difference. Please ignore this comment.

When I tried doing this, VS complained about not understanding the [Intrinsic] attribute. I didn't bother investigating why, but I did see some commented out code in this file with similar [Intrinsic] attributes. Maybe that's the reason @GrabYourPitchforks didn't include the whole file?

I have no problem with including the PNSE file as long as we're not introducing public API. We'd need to define a dummy [Intrinsic] attribute in the consuming project, but that's very straightforward.

Ah, that would be an issue. The types in the PNSE file are public, not internal.

I wonder if we should have ifdefs around these, just like how we have ifdefs around some of the new code analysis attributes. It might help code sharing between libraries that target netstandard2.0.
(Really that's an issue for a separate PR, but I wonder if this PR is a forcing function for it.)

It actually sounds like we need one for some of the other OOB libraries as well. It might be good to just have an official shim library.

System.Text.Encodings.Web is using ifdefs today, but multitargets netstandard2.0, netcoreapp3.1, and net5; so the ifdefs are going to get a bit crazier

For System.Text.Encodings.Web, I resorted to copying this file and extending it as needed. But would be great to have a package for frictionless intrinsics API usage.

src/libraries/System.Utf8String.Experimental/src/System/Runtime/Intrinsics/Intrinsics.Shims.cs

src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs

...ies/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/AdvSimd.PlatformNotSupported.cs

src/libraries/Directory.Build.targets

pgovind · 2020-07-09T23:45:17Z

Thanks for the stamp Tanner. FWIW, here are the micro benchmarks I ran on my machine (Yeeesh, that took an unreasonable amount of time to set up):

D:\repos\performance\src\tools\ResultsComparer>dotnet run --base "........\before_arm64" --diff "........\after_arm64" --threshold 0.01%
summary:
better: 1, geomean: 2.057
total diff: 1

No Slower results for the provided threshold = 0.01% and noise filter = 0.3ns.

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Text.Experimental.Perf.ToUtf16(expected: "This is a big string of words.	2.06	1207.03	586.81

kunalspathak · 2020-07-10T01:00:47Z

src/libraries/System.Utf8String.Experimental/src/System/Runtime/Intrinsics/Intrinsics.Shims.cs

        public static T GetElement<T>(this Vector128<T> vector, int index) where T : struct => throw new PlatformNotSupportedException();
        public static T ToScalar<T>(this Vector64<T> vector) where T : struct => throw new PlatformNotSupportedException();
+        public static unsafe Vector128<ulong> CreateScalar(ulong value) => throw new PlatformNotSupportedException();
+        public static T ToScalar<T>(this Vector128<T> vector) where T : struct => throw new PlatformNotSupportedException();


I don't see the usage of these APIs inside ASCIIUtility.cs. Any reason why this is added?

Hmm, it looks like they sneaked in while I was doing an interactive add. I need them for my next PR, so I think I'll leave them in, unless someone disagrees :)

eiriktsarpalis · 2020-07-10T13:05:04Z

src/libraries/Directory.Build.targets

          Returns="$(DocumentationFile)"/>

+  <PropertyGroup>
+    <DefineConstants Condition="'$(IsPrerelease)' != 'false' and '$(TargetFramework)' != '$(NetCoreAppCurrent)'">$(DefineConstants),USE_INTERNAL_ACCESSIBILITY</DefineConstants>


I'm not sure I like this living in Directory.Build.props. Shouldn't it rather be something that individual projects opt into?

That was my first reaction too. Levi suggested that it live here, so I didn't pay too much attention to it. In my next PR, I'll move it to System.Utf8Experimental.csproj.

Is this constant meant to be used in contexts other than intrinsics shims? If not, then we should probably use a more specific name like USE_INTERNAL_INTRINSICS_SHIMS or something.

From an infrastructure standpoint this is wrong as the TargetFramework property shouldn't be read outside the project file. Instead inferred properties like TargetFrameworkVersion and TargetFrameworkIdentifier should be used. If you already plan to move it then it's fine.

eiriktsarpalis · 2020-07-10T21:45:26Z

...ies/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/ArmBase.PlatformNotSupported.cs

    /// </summary>
    [CLSCompliant(false)]
-    public abstract class ArmBase
+#if USE_INTERNAL_ACCESSIBILITY


I think it would make sense to remove the CLSCompliant attribute when USE_INTERNAL_ACCESSIBILITY is enabled. Getting build warnings which I have to explicitly disable.

Ah yes. Will submit a small PR for this shortly

pgovind added the area-System.Runtime.Intrinsics label Jun 30, 2020

pgovind requested review from GrabYourPitchforks and tannergooding July 1, 2020 04:15

pgovind mentioned this pull request Jul 1, 2020

AdvSimd support for System.Text.Unicode.Utf8Utility.GetPointerToFirstInvalidByte #38653

Merged

GrabYourPitchforks reviewed Jul 1, 2020

View reviewed changes

tannergooding reviewed Jul 1, 2020

View reviewed changes

carlossanlop reviewed Jul 1, 2020

View reviewed changes

src/libraries/System.Utf8String.Experimental/src/System/Runtime/Intrinsics/Intrinsics.Shims.cs Outdated Show resolved Hide resolved

carlossanlop reviewed Jul 1, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Text/ASCIIUtility.cs Outdated Show resolved Hide resolved

pgovind force-pushed the arm64 branch from d09963d to e51075d Compare July 6, 2020 18:02

tannergooding reviewed Jul 6, 2020

View reviewed changes

...ies/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/AdvSimd.PlatformNotSupported.cs Outdated Show resolved Hide resolved

pgovind commented Jul 7, 2020

View reviewed changes

src/libraries/Directory.Build.targets Outdated Show resolved Hide resolved

pgovind force-pushed the arm64 branch from b4cbd63 to 3fbe80f Compare July 7, 2020 17:04

safern reviewed Jul 7, 2020

View reviewed changes

src/libraries/Directory.Build.targets Outdated Show resolved Hide resolved

safern reviewed Jul 7, 2020

View reviewed changes

src/libraries/Directory.Build.targets Outdated Show resolved Hide resolved

Intrinsics support for WidenFourAsciiBytesToUtf16AndWriteToBuffer

068756b

pgovind force-pushed the arm64 branch from 89236e5 to 068756b Compare July 9, 2020 20:22

tannergooding approved these changes Jul 9, 2020

View reviewed changes

kunalspathak reviewed Jul 10, 2020

View reviewed changes

eiriktsarpalis reviewed Jul 10, 2020

View reviewed changes

pgovind merged commit 7fab504 into dotnet:master Jul 10, 2020

pgovind deleted the arm64 branch July 10, 2020 18:20

eiriktsarpalis reviewed Jul 10, 2020

View reviewed changes

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Conversation

pgovind commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GrabYourPitchforks commented Jul 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlossanlop Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pgovind commented Jul 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgovind Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

pgovind commented Jun 30, 2020 •

edited

Loading

carlossanlop Jul 1, 2020 •

edited

Loading

pgovind Jul 10, 2020 •

edited

Loading