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.

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

Add ARM version of adler32 #2015

Merged
merged 33 commits into from
Mar 2, 2022
Merged

Add ARM version of adler32 #2015

merged 33 commits into from
Mar 2, 2022

Conversation

brianpopow
Copy link
Collaborator

@brianpopow brianpopow commented Feb 18, 2022

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

I was experimenting a bit with ARM hardware intrinsics and decided to port the adler32 version of chromium to ImageSharp.

BenchmarkDotNet=v0.13.1, OS=ubuntu 20.04
Unknown processor
.NET SDK=5.0.402
[Host] : .NET 5.0.11 (5.0.1121.47308), Arm64 RyuJIT
DefaultJob : .NET 5.0.11 (5.0.1121.47308), Arm64 RyuJIT

Method Mean Error StdDev
AdlerARM 1.082 us 0.0123 us 0.0109 us
AdlerScalar 3.823 us 0.0165 us 0.0146 us

CPU info:

Architecture:        aarch64
CPU op-mode(s):      32-bit, 64-bit
Byte Order:          Little Endian
CPU(s):              6
On-line CPU(s) list: 0-5
Thread(s) per core:  1
Core(s) per socket:  3
Socket(s):           2
Vendor ID:           ARM
Model:               4
Model name:          Cortex-A53
Stepping:            r0p4
CPU max MHz:         1896.0000
CPU min MHz:         100.0000
BogoMIPS:            48.00
Flags:               fp asimd evtstrm aes pmull sha1 sha2 crc32

Benchmark was run with the following data:

int N = 4096;
byte[] data = new byte[N];
new Random(1).NextBytes(data);

There is still a bug, adler results for N 4096: fixed

Arm: 1010560139
Scalar: 1010560140

Copy link
Contributor

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

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

For .NET 7 cross-platform intrinsics will be available, so that code can be written on behalf of Vector128<T> and not via Sse2 / AdvSimd.

I know that it's still a long time until is .NET 7 is available and adopted, but is this something that should be focused on now already?

src/ImageSharp/Compression/Zlib/Adler32.cs Outdated Show resolved Hide resolved
src/ImageSharp/Compression/Zlib/Adler32.cs Outdated Show resolved Hide resolved
src/ImageSharp/Compression/Zlib/Adler32.cs Outdated Show resolved Hide resolved
@brianpopow
Copy link
Collaborator Author

For .NET 7 cross-platform intrinsics will be available, so that code can be written on behalf of Vector128<T> and not via Sse2 / AdvSimd.

I know that it's still a long time until is .NET 7 is available and adopted, but is this something that should be focused on now already?

That's interesting, I was not aware of this. How would that work? SSE has intrinsics which ARM does not have and vice versa.

I think its to early to early to consider this now. Anything ARM related is now only pretty much experimental and will not land in the main branch anytime soon.

@gfoidl
Copy link
Contributor

gfoidl commented Feb 19, 2022

How would that work? SSE has intrinsics which ARM does not have and vice versa

Only the "common subset" is exposes as xplat-intrinsics. The journey started in dotnet/runtime#49397 but more operations got added so far -- I can't find a up-to-date summary of all operations 😢.
For the special cases one needs to fall back to the current intrinsics style (Sse.IsSupported / AdvSimd.IsSupported).

I think its to early to early to consider this now

This is also my expectation. Just wanted to point to it (as a reminder that something is on the way).

@brianpopow
Copy link
Collaborator Author

This is also my expectation. Just wanted to point to it (as a reminder that something is on the way).

Thanks for sharing, that's very interesting to know.

@brianpopow brianpopow changed the title WIP: Add ARM version of adler32 Add ARM version of adler32 Feb 21, 2022
@antonfirsov
Copy link
Member

I know that it's still a long time until is .NET 7 is available and adopted, but is this something that should be focused on now already?

No, since - unlike the BCL - we have to maintain multiple .NET versions backwards, not just the shiniest-latest one, which means we would end up complicating our code with lots of #if-s instead of simplifying it. These are cool features for dotnet/runtime itself, but radically less useful for 3rd libraries until .NET 6 goes out of support.

@gfoidl
Copy link
Contributor

gfoidl commented Feb 22, 2022

@antonfirsov I partly agree.

Total agreement for backwards-compat and what you said about the BCL.

For the case of ARM I neither agree nor disagree. ARM isn't supported right now, so no problems with backwards-compat, hence this opens the opportunity to use one code path with the xplat-intrinsics for ARM and x86/x64. '#if'def would then only be for NET7_0_OR_GREATER.

Right now it looks like non-BCL libraries have to go special routes / code-pathes for each type of HW-platform, then with .NET 7 (and when .NET 6 isn't supported anymore) these code-pathes can be merged by using the xplat-intrinsics.

So if ARM should be supported by .NET 6 the different code-pathes is IMO the right way to go.
If ARM support is sufficient to have for .NET 7 onwards, the one should go with the xplat-intrinsics.

But it's relatively new concept, so it has to evolve and so it's good to have some discussion here to see what's doable and what's not.

@brianpopow brianpopow changed the base branch from main to arm February 26, 2022 22:00
@brianpopow
Copy link
Collaborator Author

I will go ahead and merge this into the arm branch

@brianpopow brianpopow merged commit fe29201 into arm Mar 2, 2022
@brianpopow brianpopow deleted the bp/adlerarm branch March 2, 2022 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants