-
-
Notifications
You must be signed in to change notification settings - Fork 862
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
Add Arm intrinsics to JpegColorConverter RGB #2397
Conversation
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. |
|
nice, I wasn't sure at first that this will beat the |
Our 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) Sdk: 7.0.x |
Yeah, changing the DotNet Setup task to use 7 (or 7 and 6) might work. |
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 |
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... |
src/ImageSharp/Formats/Jpeg/Components/ColorConverters/JpegColorConverter.RgbArm.cs
Outdated
Show resolved
Hide resolved
…orConverter.RgbArm.cs Co-authored-by: Günther Foidl <gue@korporal.at>
There was a problem hiding this 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!
I think we missed here adjusting the JpegColorConverterTests for ARM. It should now also run the tests with |
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 I think the following IntrinsicsConfig should now be used:
On x64, we will execute the scalar version with |
as suggested in SixLabors#2397 (comment)
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: This will do the following on x64, assuming AVX is available for the CPU:
On ARM:
Thats why I suggested 'HwIntrinsics.AllowAll | HwIntrinsics.DisableHWIntrinsic' |
Prerequisites
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