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

Explore adding an ISimdVector<TSelf, T> interface #76423

Closed
wants to merge 2 commits into from

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented Sep 30, 2022

This is a basic attempt at exploring: #76244

The first commit defines an ISimdVector interface and implements it on the relevant Vector64/128/256<T> types. It does not implement it on Vector<T> yet nor is it "complete" (several APIs were left "TODO").

The second commit uses these to simplify the LastIndexOfValueType implementations

CC. @stephentoub, @jeffhandley

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Sep 30, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics
See info in area-owners.md if you want to be subscribed.

Issue Details

This is a basic attempt at exploring: #76244

CC. @stephentoub, @jeffhandley

Author: tannergooding
Assignees: tannergooding
Labels:

area-System.Runtime.Intrinsics, new-api-needs-documentation

Milestone: -

/// <summary>Defines a single instruction, multiple data (SIMD) vector type.</summary>
/// <typeparam name="TSelf">The type that implements the interface.</typeparam>
/// <typeparam name="T">The type of the elements in the vector.</typeparam>
internal interface ISimdVector<TSelf, T>
Copy link
Member Author

Choose a reason for hiding this comment

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

I went with ISimdVector to avoid conflicting with the more general IVector that might be desirable for ML or Tensor like scenarios.

Copy link
Member Author

@tannergooding tannergooding Sep 30, 2022

Choose a reason for hiding this comment

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

This is also a place where "higher kinded types" would be beneficial.

It would allow us to specify that TSelf is itself generic and put constraints on that required generic parameter (e.g. TSelf<T>). That would in turn make ISimdVector "less verbose" and make consumption "easier" as users have to specify, take, and track less info.

Comment on lines +14 to +31
: IAdditionOperators<TSelf, TSelf, TSelf>,
// IAdditiveIdentity<TSelf, TSelf>,
IBitwiseOperators<TSelf, TSelf, TSelf>,
// IComparisonOperators<TSelf, TSelf, bool>,
// IDecrementOperators<TSelf>,
IDivisionOperators<TSelf, TSelf, TSelf>,
IEqualityOperators<TSelf, TSelf, bool>,
IEquatable<TSelf>,
// IIncrementOperators<TSelf>,
// IMinMaxValue<TSelf>,
// IModulusOperators<TSelf, TSelf, TSelf>,
// IMultiplicativeIdentity<TSelf, TSelf>,
IMultiplyOperators<TSelf, TSelf, TSelf>,
// IShiftOperators<TSelf, int, TSelf>
// ISpanFormattable,
ISubtractionOperators<TSelf, TSelf, TSelf>,
IUnaryNegationOperators<TSelf, TSelf>,
IUnaryPlusOperators<TSelf, TSelf>
Copy link
Member Author

Choose a reason for hiding this comment

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

The commented out APIs would expose "more" than what the Vector64/128/256<T> types already do. We should likely explore whether those APIs make sense (I expect in many cases they will).

For cases like IAdditionOperators this adds operator checked + and doesn't give it a "correct" implementation. We could do so and it may even be desirable to do that longer term.

Comment on lines +32 to +33
where TSelf : unmanaged, ISimdVector<TSelf, T>
where T : struct
Copy link
Member Author

Choose a reason for hiding this comment

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

These may be "more restrictive" than necessary.

In the case of where TSelf : unmanaged it gives as rationale behavior for reinterpret casts and other behaviors so we can provide default implementations for several methods. Once it could contain a reference type (such as from a 3rd party type implementing these) many assumptions go out the window.

In the case of where T : struct it just matches the current constraint on the Vector64/128/256<T> types.

/// <exception cref="ArgumentOutOfRangeException">The length of <paramref name="values" /> is less than <see cref="Count" />.</exception>
/// <exception cref="NotSupportedException">The type of the elements in the vector (<typeparamref name="T" />) is not supported.</exception>
/// <exception cref="NullReferenceException"><paramref name="values" /> is <c>null</c>.</exception>
static virtual TSelf Create(T[] values)
Copy link
Member Author

Choose a reason for hiding this comment

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

These interfaces will be "more similar to" Vector<T> due to it being an abstraction and not "fixed sized".

This means you can't do something like Create(float e0, float e1, float e2, float e3) and that may be "restrictive" to some scenarios.

The "workaround" would be for users to pass in such values to the generic implementation (e.g. SimdImpl<Vector128<TValue>(..., Vector128.Create(...)))

// TODO: Dot
// TODO: ExtractMostSignificantBits
// TODO: Floor
// TODO: Load
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we can't constrain where T : unmanaged we can't expose Load(T* source). At best we could expose Load(void* source) instead. This is probably fine, just something to consider.


// TODO: static abstract operator *(TSelf left, T right);
// TODO: As<TTo>()
// TODO: Ceiling()
Copy link
Member Author

@tannergooding tannergooding Sep 30, 2022

Choose a reason for hiding this comment

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

Ceiling and Floor are only exposed for float/double. We could provide implementations that "nop" for integers.

APIs like ConvertToDouble/Int32/Int64/Single/UInt32/UInt64 likewise only support a subset of T and couldn't readily be exposed since allowing "any" type has strange semantics due to some being "narrowing/widening" operations and therefore changing the count.

// TODO: As<TTo>()
// TODO: Ceiling()
// TODO: Dot
// TODO: ExtractMostSignificantBits
Copy link
Member Author

Choose a reason for hiding this comment

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

Much like Vector<T>, this can't be exposed like it is on Vector64/128/256<T>

Instead, we'd have to expose this as returning some VectorMask<T>. That then complicates things quite a bit since its another piece of info required to "use" the type. If .NET had "associated types", this would be "nicer".

Comment on lines +440 to +442
// TODO: ShiftLeft
// TODO: ShiftRightArithmetic
// TODO: ShiftRightLogical
Copy link
Member Author

Choose a reason for hiding this comment

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

These are ones that should "probably" also have operators exposed, but where we don't have them today. Just the named methods.

Comment on lines +16 to +18
public static void CopyTo<TVector, T>(this TVector vector, T[] destination)
where TVector : unmanaged, ISimdVector<TVector, T>
where T : struct
Copy link
Member Author

Choose a reason for hiding this comment

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

This is being "clever" to continue allowing these as extension methods for perf reasons (instance methods on value types take the first parameter as a hidden byref, which is undesirable for types that should always be enregistered and almost never address taken).


[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static int ComputeLastIndex<T>(nint offset, Vector256<T> equals) where T : struct
private static int ComputeLastIndex<TVector, T>(nint offset, TVector equals)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is working around the lack of variable sized ExtractMostSignificantBits.

It's not pretty, but it works and ideally gets replaced in .NET 8 as part of the VectorMask work...

return SimdImpl<Vector128<TValue>>(ref searchSpace, value, length);
}

[MethodImpl(MethodImplOptions.AggressiveOptimization)]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this AggressiveOptimization (and not AggressiveInlining)?
I think AggressiveOptimization prevents tiering, PGO, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy/paste from the parent. It likely should be AI+AO instead.

@adamsitnik
Copy link
Member

The second commit uses these to simplify the LastIndexOfValueType implementations

From this perspective it looks very promising. With such an interface I can see us adding Vector512 code paths way quicker and easier (but ideally we would use just Vector<T> if that is possible).

@ghost ghost closed this Oct 30, 2022
@ghost
Copy link

ghost commented Oct 30, 2022

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 29, 2022
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants