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 intrinsics to JpegColorConverter RGB #2397

Conversation

stefannikolei
Copy link
Contributor

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

This is my first attempt in learning something about intrinsics.
So I picked something which looked straight forward (for me) for a port to arm.

I will also try to see if there is already an benchmark for this, or going to look into writing one

@brianpopow
Copy link
Collaborator

I will also try to see if there is already an benchmark for this, or going to look into writing one

There is a benchmark comparing the AVX version to the RgbVector version in Codecs/Jpeg/ColorConverion/RgbColorConversion.cs

You can make a similar one for the ARM version.

@stefannikolei
Copy link
Contributor Author

Method Mean Error StdDev Ratio
Scalar 323.78 ns 0.979 ns 0.916 ns 1.00
SimdVector8 110.25 ns 1.073 ns 0.951 ns 0.34
SimdVectorArm 90.27 ns 1.841 ns 2.581 ns 0.28

@brianpopow
Copy link
Collaborator

Method Mean Error StdDev Ratio
Scalar 323.78 ns 0.979 ns 0.916 ns 1.00
SimdVector8 110.25 ns 1.073 ns 0.951 ns 0.34
SimdVectorArm 90.27 ns 1.841 ns 2.581 ns 0.28

nice, I wasn't sure at first that this will beat the RgbVector version, but this benchmark looks good!

@JimBobSquarePants
Copy link
Member

Our UnscopedRefAttribute is not working on the ARM CI server.

It looks like they're using a different version of MSBuild

MSBuild version 17.3.2+561848881 for .NET

Verses GitHub's 17.5.0-preview-23061-01+040e2a90e

@stefannikolei
Copy link
Contributor Author

Our UnscopedRefAttribute is not working on the ARM CI server.

It looks like they're using a different version of MSBuild

MSBuild version 17.3.2+561848881 for .NET

Verses GitHub's 17.5.0-preview-23061-01+040e2a90e

When I look at it, they do it right. 17.3 is the expected MSBuild version for net6 (https://learn.microsoft.com/de-de/dotnet/core/porting/versioning-sdk-msbuild-vs)
It will probably work when we define this build task like this:

Sdk: 7.0.x
Framework: 6.0.x

@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Mar 13, 2023

Yeah, changing the DotNet Setup task to use 7 (or 7 and 6) might work.

@JimBobSquarePants
Copy link
Member

I think we're hitting issues in the tests because the VMs are underpowered. We can bump them up by changing the signature.

Let's try buildjet-8vcpu-ubuntu-2204-arm first and if that doesn't help, bump it up again.
https://buildjet.com/for-github-actions/docs/runners#arm

@stefannikolei
Copy link
Contributor Author

stefannikolei commented Mar 13, 2023

I think we're hitting issues in the tests because the VMs are underpowered. We can bump them up by changing the signature.

Let's try buildjet-8vcpu-ubuntu-2204-arm first and if that doesn't help, bump it up again. https://buildjet.com/for-github-actions/docs/runners#arm

I removed the .net6 target for arm. But now hitting another issue god dammit...

Update. Libgdi+ was not installed because it looked for an image 4vcpu and not 8vcpu...

…orConverter.RgbArm.cs

Co-authored-by: Günther Foidl <gue@korporal.at>
Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

This is great. Thanks!

@JimBobSquarePants JimBobSquarePants merged commit 5283d77 into SixLabors:main Mar 13, 2023
@brianpopow
Copy link
Collaborator

I think we missed here adjusting the JpegColorConverterTests for ARM. It should now also run the tests with DisableArm64AdvSimd, right?

@stefannikolei
Copy link
Contributor Author

I think we missed here adjusting the JpegColorConverterTests for ARM. It should now also run the tests with DisableArm64AdvSimd, right?

I can add that in the new pr I made. But what does that do? It will then run some tests instead of 2 times 3 times and disables specific simds

@brianpopow
Copy link
Collaborator

I think we missed here adjusting the JpegColorConverterTests for ARM. It should now also run the tests with DisableArm64AdvSimd, right?

I can add that in the new pr I made. But what does that do? It will then run some tests instead of 2 times 3 times and disables specific simds

This will run the tests using FeatureTestRunner, which as you noticed let you disable hardware intrinsics features. The point of this is to test the Scalar path and the intrinsics path. Now on the ARM CI (assuming the CI supports intrinsics), only the intrinsics path is tested.

I think the following IntrinsicsConfig should now be used:

private const HwIntrinsics IntrinsicsConfig = HwIntrinsics.AllowAll | HwIntrinsics.DisableHWIntrinsic;

On x64, we will execute the scalar version with HwIntrinsics.DisableHWIntrinsic and execute the AVX version with HwIntrinsics.AllowAll.
On ARM, we will execute the scalar version with HwIntrinsics.DisableHWIntrinsic and execute the hardware intrinsics version with HwIntrinsics.AllowAll.

stefannikolei added a commit to stefannikolei/ImageSharp that referenced this pull request Mar 13, 2023
@JimBobSquarePants
Copy link
Member

Do we not have various granular implementations though? Avx, Sse etc? We want to turn them off incrementally not en-mass.

@brianpopow
Copy link
Collaborator

Do we not have various granular implementations though? Avx, Sse etc? We want to turn them off incrementally not en-mass.

That's in general true, but we only have AVX version for x64 CPU's, no SSE variants for the color conversion.

The alternative would be: HwIntrinsics.AllowAll | HwIntrinsics.DisableAVX | HwIntrinsics.DisableArm64AdvSimd (right?)

This will do the following on x64, assuming AVX is available for the CPU:

  • HwIntrinsics.AllowAll: Run AVX version
  • HwIntrinsics.DisableAVX: Run Scalar version
  • HwIntrinsics.DisableArm64AdvSimd: Again run AVX version (correct?)

On ARM:

  • HwIntrinsics.AllowAll: Run ARM version
  • HwIntrinsics.DisableAVX: Run ARM version
  • HwIntrinsics.DisableArm64AdvSimd: Run Scalar version

Thats why I suggested 'HwIntrinsics.AllowAll | HwIntrinsics.DisableHWIntrinsic'

@stefannikolei stefannikolei deleted the stefannikolei/jpegcolorconverter_arm branch March 25, 2023 16:31
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