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

Use compile time constants over method for all MMShuffle operations. #2365

Merged
merged 4 commits into from
Feb 22, 2023

Conversation

JimBobSquarePants
Copy link
Member

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

Fixes #2121

Ratios do seem to be better for Avx and Sse operations.

CC @gfoidl @tannergooding

@JimBobSquarePants
Copy link
Member Author

This is super useful coming in .NET 8!

image

@JimBobSquarePants JimBobSquarePants merged commit 75121b2 into main Feb 22, 2023
@JimBobSquarePants JimBobSquarePants deleted the js/2121 branch February 22, 2023 13:31
@stefannikolei
Copy link
Contributor

stefannikolei commented Feb 24, 2023

@JimBobSquarePants It seems that this PR gives me some issues.

I tried to execute the tests after this PR and over 1000 failed.
I checked out an commit before this PR and all tests went green.

I am seeing those Exception:

LoadResizeSave [SKIP]
[xUnit.net 00:00:05.77]       Profiling benchmark, enable manually!
[xUnit.net 00:00:05.93]     BulkShuffle4Slice3Channel(count: 4) [FAIL]
[xUnit.net 00:00:05.93]       Microsoft.DotNet.RemoteExecutor.RemoteExecutionException : Remote process failed with an unhandled exception.
[xUnit.net 00:00:05.93]       Stack Trace:
[xUnit.net 00:00:05.93]         
[xUnit.net 00:00:05.93]         Child exception:
[xUnit.net 00:00:05.93]           System.ArgumentOutOfRangeException: Value 156 must be greater than or equal to 0 and less than or equal to 3. (Parameter 'control')
[xUnit.net 00:00:06.07]         /Users/stefannikolei/projects/github/sixlabors/ImageSharp/shared-infrastructure/src/SharedInfrastructure/DebugGuard.cs(279,0): at SixLabors.DebugGuard.ThrowArgumentOutOfRangeException(String parameterName, String message)
[xUnit.net 00:00:06.07]         /Users/stefannikolei/projects/github/sixlabors/ImageSharp/shared-infrastructure/src/SharedInfrastructure/DebugGuard.cs(151,0): at SixLabors.DebugGuard.MustBeBetweenOrEqualTo[TValue](TValue value, TValue min, TValue max, String parameterName)
[xUnit.net 00:00:06.07]         /Users/stefannikolei/projects/github/sixlabors/ImageSharp/src/ImageSharp/Common/Helpers/Shuffle/IShuffle4Slice3.cs(19,0): at SixLabors.ImageSharp.DefaultShuffle4Slice3..ctor(Byte control)
[xUnit.net 00:00:06.07]         /Users/stefannikolei/projects/github/sixlabors/ImageSharp/tests/ImageSharp.Tests/Common/SimdUtilsTests.Shuffle.cs(459,0): at SixLabors.ImageSharp.Tests.Common.SimdUtilsTests.<BulkShuffle4Slice3Channel>g__RunTest|41_0(String serialized)

My environment:

SixLabors.ImageSharp.Tests: BenchmarkDotNet=v0.13.0, OS=macOS 13.2.1 (22D68) [Darwin 22.3.0]
Intel Core i9-9880H CPU 2.30GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK=7.0.200
  [Host] : .NET 6.0.14 (6.0.1423.7309), X64 RyuJIT

When I run the tests in Release all tests pass. Running it in Debug fails the tests for me.

@JimBobSquarePants
Copy link
Member Author

You’re right! It’s a debug guard. Thanks!

@JimBobSquarePants
Copy link
Member Author

Have pushed a fix in #2369

@stefannikolei
Copy link
Contributor

Have pushed a fix in #2369

Perhaps a good idea to run the tests in the CI pipeline in Debug mode?

@JimBobSquarePants
Copy link
Member Author

We’d maybe run a single entry in the matrix. I wouldn’t run more than that as it gets expensive. We want to preserve our release mode ones also to ensure we catch jit issues

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.

Vector{128,256} operations that use MmShuffle fall back to method call
3 participants