Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BitOperations arm64 intrinsic for LeadingZeroCount, TrailingZeroCount and Log2 #34486

Merged
merged 5 commits into from
Apr 4, 2020
Merged

BitOperations arm64 intrinsic for LeadingZeroCount, TrailingZeroCount and Log2 #34486

merged 5 commits into from
Apr 4, 2020

Conversation

kunalspathak
Copy link
Member

@kunalspathak kunalspathak commented Apr 3, 2020 •
edited
Loading

Use arm64 intrinsics for BitOperations's LeadingZeroCount, TrailingZeroCount and Log2 methods.

Below are the performance improvement details of each API.

API before (in us) after (in us) % diff
LeadingZeroCount_uint 6.538 1.544 76%
LeadingZeroCount_ulong 6.614 1.544 77%
Log2_uint 5.774 1.920 67%
Log2_ulong 6.925 1.921 72%
TrailingZeroCount_uint 2.722 1.665 39%
TrailingZeroCount_ulong 2.891 1.666 42%

I didn't include PopCount() as part of this PR because it needs special handling of CreateScalar in JIT. Once #34485 is fixed, I will send a separate PR for it PopCount().

Contributes to #33308 and #33495 .

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. Please help me learn by adding exactly one area label.

@kunalspathak
Copy link
Member Author

kunalspathak commented Apr 3, 2020 •
edited
Loading

Please take a look @dotnet/jit-contrib , @tannergooding

@BruceForstall
Copy link
Member

The Log2_uint improvement % number looks incorrect.

For these arm64-specific libraries changes, I think you need to trigger the runtime AzDO pipeline manually, as the default PR testing does not include libraries arm64 testing.

@kunalspathak
Copy link
Member Author

@BruceForstall - Triggered one just now at https://dev.azure.com/dnceng/public/_build/results?buildId=587055&view=results
I do see jobs "Libraries Test Run release coreclr Linux arm64 Release" in it. So hopefully that is the one we wanted.

@kunalspathak kunalspathak force-pushed the bitoperations-intrinsic branch from b628d47 to a2effc7 Compare April 3, 2020 05:58
@kunalspathak
Copy link
Member Author

I was seeing ARM not present in System.Runtime.Intrinsics namespace errors. Not sure if this was because I didn't had #34107.

I have rebased my changes and triggered another pipeline run.
https://dev.azure.com/dnceng/public/_build/results?buildId=587081&view=results

@kunalspathak
Copy link
Member Author

Jobs are failing with following errors. Do i need to add a package reference or something inside System.Utf8String.Experimental.csproj. How is it suppose to work?

F:\workspace\_work\1\s\src\libraries\System.Private.CoreLib\src\System\Numerics\BitOperations.cs(8,33): error CS0234: The type or namespace name 'Arm' does not exist in the namespace 'System.Runtime.Intrinsics' (are you missing an assembly reference?) [F:\workspace\_work\1\s\src\libraries\System.Utf8String.Experimental\src\System.Utf8String.Experimental.csproj]
##[error]src\libraries\System.Private.CoreLib\src\System\Numerics\BitOperations.cs(8,33): error CS0234: The type or namespace name 'Arm' does not exist in the namespace 'System.Runtime.Intrinsics' (are you missing an assembly reference?)

@tannergooding , @echesakovMSFT

@EgorBo
Copy link
Member

EgorBo commented Apr 3, 2020

What about BitOperations.RotateLeft does jit lower GT_ROR to arm's ror ?

@kunalspathak
Copy link
Member Author

What about BitOperations.RotateLeft does jit lower GT_ROR to arm's ror ?

Yes. It converts RotateRight to ror and RotateLeft to neg/ror because there is no rol instruction in arm.

echesakov
echesakov approved these changes Apr 3, 2020
View reviewed changes
Copy link
Contributor

@echesakov echesakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with comments regarding redundant casts to int

@echesakov
Copy link
Contributor

Jobs are failing with following errors. Do i need to add a package reference or something inside System.Utf8String.Experimental.csproj. How is it suppose to work?
F:\workspace_work\1\s\src\libraries\System.Private.CoreLib\src\System\Numerics\BitOperations.cs(8,33): error CS0234: The type or namespace name 'Arm' does not exist in the namespace 'System.Runtime.Intrinsics' (are you missing an assembly reference?) [F:\workspace_work\1\s\src\libraries\System.Utf8String.Experimental\src\System.Utf8String.Experimental.csproj]
##[error]src\libraries\System.Private.CoreLib\src\System\Numerics\BitOperations.cs(8,33): error CS0234: The type or namespace name 'Arm' does not exist in the namespace 'System.Runtime.Intrinsics' (are you missing an assembly reference?)

@kunalspathak My understanding that you need to update Intrinsics.Shims.cs simiralrly as it's been done for System.Runtime.Intrinsics.X86 in order for this to work - @tannergooding is it right?

@tannergooding
Copy link
Member

Jobs are failing with following errors. Do i need to add a package reference or something inside System.Utf8String.Experimental.csproj. How is it suppose to work?
My understanding that you need to update Intrinsics.Shims.cs simiralrly as it's been done for System.Runtime.Intrinsics.X86 in order for this to work - @tannergooding is it right?

I believe that is the case, but CC. @GrabYourPitchforks since I think he "owns" System.Utf8String.Experimental.
Just as a note, we don't want to do this more broadly as we don't want to have a reference to the experiemental package from non-experimental packages.
If there is anything non-experimental that isn't in S.P.Corelib that would require a similar change, we may need to wait for the bits to be moved in box (which should be happening once API review on everything finishes).

@kunalspathak
Copy link
Member Author

Jobs are failing with following errors. Do i need to add a package reference or something inside System.Utf8String.Experimental.csproj. How is it suppose to work?
F:\workspace_work\1\s\src\libraries\System.Private.CoreLib\src\System\Numerics\BitOperations.cs(8,33): error CS0234: The type or namespace name 'Arm' does not exist in the namespace 'System.Runtime.Intrinsics' (are you missing an assembly reference?) [F:\workspace_work\1\s\src\libraries\System.Utf8String.Experimental\src\System.Utf8String.Experimental.csproj]
##[error]src\libraries\System.Private.CoreLib\src\System\Numerics\BitOperations.cs(8,33): error CS0234: The type or namespace name 'Arm' does not exist in the namespace 'System.Runtime.Intrinsics' (are you missing an assembly reference?)

@kunalspathak My understanding that you need to update Intrinsics.Shims.cs simiralrly as it's been done for System.Runtime.Intrinsics.X86 in order for this to work - @tannergooding is it right?

That seemed to have solved the problem (atleast on my local machine). Thanks!

@kunalspathak
Copy link
Member Author

The Log2_uint improvement % number looks incorrect.

@BruceForstall - This measurements were taken on ARM64 machine manually for MicroBenchmarks. Changes are here: dotnet/performance#1264

@kunalspathak
Copy link
Member Author

Failures related to #34465
//cc @jaredpar

@kunalspathak kunalspathak merged commit 510efdf into dotnet:master Apr 4, 2020
@kunalspathak kunalspathak deleted the bitoperations-intrinsic branch April 4, 2020 00:27
@BruceForstall
Copy link
Member

@BruceForstall - This measurements were taken on ARM64 machine manually for MicroBenchmarks.

given this:

API before (in us) after (in us) % diff
Log2_uint 5.774 1.920 15%
Log2_ulong 6.925 1.921 72%

It seems like Log2_unit should be 67% improvement, not 15%.

@kunalspathak
Copy link
Member Author

It seems like Log2_unit should be 67% improvement, not 15%.

Ah yes.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@echesakov echesakov echesakov approved these changes

@tannergooding tannergooding tannergooding approved these changes

@BruceForstall BruceForstall BruceForstall approved these changes

Assignees
No one assigned
Labels
arch-arm64 area-System.Numerics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kunalspathak @Dotnet-GitSync-Bot @BruceForstall @EgorBo @echesakov @tannergooding