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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create vector constants inline and not via ROS<byte> #2419

Merged
merged 4 commits into from Mar 30, 2023

Conversation

gfoidl
Copy link
Contributor

@gfoidl gfoidl commented Mar 26, 2023

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

On .NET 6 onwards (which is relevant here) it's better for codegen to define vector constant inline (including to have helper-methods for this).

  • private static readonly VectorXYZ-fields produce less ideal code with more indirections
  • reading the vector constant from a ROS (which may refer directly to assembly's data segment) results in some overhead

Now the vector constants can be read from memory directly.

PS: I don't have any other planned PRs -- right now weather is too bad / good enough to dig through this code 馃槈

@@ -221,7 +226,7 @@ public static class HwIntrinsics
ref Vector256<float> destBase =
ref Unsafe.As<float, Vector256<float>>(ref MemoryMarshal.GetReference(dest));

nint n = (nint)(uint)(dest.Length / Vector256<float>.Count);
nint n = (nint)((uint)dest.Length / (uint)Vector256<float>.Count);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've forgotten these in the other PR (I think because they're kept as nint due the operation in L231).

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should create a helper method for these? Feels very noisy seeing this before all our SIMD loops.

internal static partial class Numerics
{
    // Not sure about the name
    public static nint VectorCount<T>(Span<T> dest, int vectorSize) => (nint)((uint)dest.Length / (uint)vectorSize);
}

So then we can do:

nint n = Numerics.VectorCount(dest, Vector256<float>.Count);

Copy link
Member

Choose a reason for hiding this comment

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

@JimBobSquarePants meh or yeah?

Copy link
Contributor Author

@gfoidl gfoidl Mar 27, 2023

Choose a reason for hiding this comment

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

I like the idea.
But it should be generic to account for different vector types w/o specifying the whole vector).

Maybe as extension to span?

Copy link
Member

Choose a reason for hiding this comment

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

But it should be generic to account for different vector types

I wish there was an IIntrinsicVector<T> interface with a static Count property but there isn't, and we are not on net7.0+ anyways :) So I'm just passing Vector256<float>.Count as a parameter in my recommendation, meaning that there is no way to check if the Vector256<T> and Span<T> element types are the same. Not ideal, but still better than writing out the verbose uint magic everywhere IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no way to check if the Vector256<T> and Span<T> element types are the same

They don't need to match. Reading e.g. Vector128<float> out of a ROS<byte> is pretty common.

So for the generic variant there could be two generic arguments. Here I don't like that the type of the span must be repeated. That should be infered (by the compiler).

In your I don't like that VectorXYZ<Type>.Count needs to be typed.

What do you think of this approach (IIRC it's only ROS/Span)?

Copy link
Member

Choose a reason for hiding this comment

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

What do you think of this approach (IIRC it's only ROS/Span)?

I like this option the most. We can have one overload for each span type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Super.

I prefer doing this in a separete PR, as it doesn't match with this PR (this location is a left-over from the other PR) and there a quite a few places that need to be touched.
Fine?

Anyway I'll do this change later this afternoon (here or in a new PR).

Copy link
Member

Choose a reason for hiding this comment

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

New PR is fine for me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--> #2422

@stefannikolei
Copy link
Contributor

@gfoidl On fire

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.

I'm really curious to benchmark the library now vs v3.0.0. There's been lots of improvements!

@JimBobSquarePants JimBobSquarePants merged commit 23a6193 into SixLabors:main Mar 30, 2023
7 checks passed
@gfoidl gfoidl deleted the vector-constants branch March 30, 2023 09:09
@gfoidl
Copy link
Contributor Author

gfoidl commented Mar 30, 2023

curious to benchmark the library now vs v3.0.0

So am I too. Memory, and especially "static footprint" should be better. The latter is important for server usage, especially for serverless computing (less startup-cost, less memory consumption).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants