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

Expose an internal ISimdVector interface and being using it to deduplicate some SIMD code #90764

Merged
merged 9 commits into from
Oct 3, 2023
14 changes: 12 additions & 2 deletions src/coreclr/jit/hwintrinsicarm64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,12 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
case NI_Vector128_Ceiling:
{
assert(sig->numArgs == 1);
assert(varTypeIsFloating(simdBaseType));

if (!varTypeIsFloating(simdBaseType))
{
retNode = impSIMDPopStack();
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for these JIT changes? Was something breaking without it? Are we missing any tests, or it's newly exposed by the interfaces and is thus implicitly tested?

Copy link
Member Author

Choose a reason for hiding this comment

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

Newly "exposed" (its still internal only) by the interface and so it's just ensuring that it remains efficient/handled.

The path previously assumed floating-point only, so we need to handle integer (which is a no-op) explicitly.


op1 = impSIMDPopStack();
retNode = gtNewSimdCeilNode(retType, op1, simdBaseJitType, simdSize);
Expand Down Expand Up @@ -1098,7 +1103,12 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
case NI_Vector128_Floor:
{
assert(sig->numArgs == 1);
assert(varTypeIsFloating(simdBaseType));

if (!varTypeIsFloating(simdBaseType))
{
retNode = impSIMDPopStack();
break;
}

op1 = impSIMDPopStack();
retNode = gtNewSimdFloorNode(retType, op1, simdBaseJitType, simdSize);
Expand Down
14 changes: 12 additions & 2 deletions src/coreclr/jit/hwintrinsicxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1371,7 +1371,12 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
case NI_Vector512_Ceiling:
{
assert(sig->numArgs == 1);
assert(varTypeIsFloating(simdBaseType));

if (!varTypeIsFloating(simdBaseType))
{
retNode = impSIMDPopStack();
break;
}

if ((simdSize < 32) && !compOpportunisticallyDependsOn(InstructionSet_SSE41))
{
Expand Down Expand Up @@ -1986,7 +1991,12 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
case NI_Vector512_Floor:
{
assert(sig->numArgs == 1);
assert(varTypeIsFloating(simdBaseType));

if (!varTypeIsFloating(simdBaseType))
{
retNode = impSIMDPopStack();
break;
}

if ((simdSize < 32) && !compOpportunisticallyDependsOn(InstructionSet_SSE41))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1013,7 +1013,9 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\UnmanagedType.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\VarEnum.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\VariantWrapper.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\Intrinsics\ISimdVector_2.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\Intrinsics\Scalar.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\Intrinsics\SimdVectorExtensions.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\Intrinsics\Vector128.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\Intrinsics\Vector128_1.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\Intrinsics\Vector128DebugView_1.cs" />
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace System.Runtime.Intrinsics
{
internal static unsafe class SimdVectorExtensions
{
// TODO: As<TFrom, TTo>

/// <summary>Copies a vector to a given array.</summary>
/// <typeparam name="TVector">The type of the vector.</typeparam>
/// <typeparam name="T">The type of the elements in the vector.</typeparam>
/// <param name="vector">The vector to be copied.</param>
/// <param name="destination">The array to which <paramref name="vector" /> is copied.</param>
/// <exception cref="ArgumentException">The length of <paramref name="destination" /> is less than <see cref="ISimdVector{TVector, T}.Count" />.</exception>
/// <exception cref="NullReferenceException"><paramref name="destination" /> is <c>null</c>.</exception>
/// <exception cref="NotSupportedException">The type of the elements in the vector (<typeparamref name="T" />) is not supported.</exception>
public static void CopyTo<TVector, T>(this TVector vector, T[] destination)
where TVector : ISimdVector<TVector, T>
{
TVector.CopyTo(vector, destination);
}

/// <summary>Copies a vector to a given array starting at the specified index.</summary>
/// <typeparam name="TVector">The type of the vector.</typeparam>
/// <typeparam name="T">The type of the elements in the vector.</typeparam>
/// <param name="vector">The vector to be copied.</param>
/// <param name="destination">The array to which <paramref name="vector" /> is copied.</param>
/// <param name="startIndex">The starting index of <paramref name="destination" /> which <paramref name="vector" /> will be copied to.</param>
/// <exception cref="ArgumentException">The length of <paramref name="destination" /> is less than <see cref="ISimdVector{TVector, T}.Count" />.</exception>
/// <exception cref="ArgumentOutOfRangeException"><paramref name="startIndex" /> is negative or greater than the length of <paramref name="destination" />.</exception>
/// <exception cref="NullReferenceException"><paramref name="destination" /> is <c>null</c>.</exception>
/// <exception cref="NotSupportedException">The type of the elements in the vector (<typeparamref name="T" />) is not supported.</exception>
public static void CopyTo<TVector, T>(this TVector vector, T[] destination, int startIndex)
where TVector : ISimdVector<TVector, T>
{
TVector.CopyTo(vector, destination, startIndex);
}

/// <summary>Copies a vector to a given span.</summary>
/// <typeparam name="TVector">The type of the vector.</typeparam>
/// <typeparam name="T">The type of the elements in the vector.</typeparam>
/// <param name="vector">The vector to be copied.</param>
/// <param name="destination">The span to which the <paramref name="vector" /> is copied.</param>
/// <exception cref="ArgumentException">The length of <paramref name="destination" /> is less than <see cref="ISimdVector{TVector, T}.Count" />.</exception>
/// <exception cref="NotSupportedException">The type of the elements in the vector (<typeparamref name="T" />) is not supported.</exception>
public static void CopyTo<TVector, T>(this TVector vector, Span<T> destination)
where TVector : ISimdVector<TVector, T>
{
TVector.CopyTo(vector, destination);
}

/// <summary>Gets the element at the specified index.</summary>
/// <typeparam name="TVector">The type of the vector.</typeparam>
/// <typeparam name="T">The type of the elements in the vector.</typeparam>
/// <param name="vector">The vector to get the element from.</param>
/// <param name="index">The index of the element to get.</param>
/// <returns>The value of the element at <paramref name="index" />.</returns>
/// <exception cref="ArgumentOutOfRangeException"><paramref name="index" /> was less than zero or greater than the number of elements.</exception>
/// <exception cref="NotSupportedException">The type of <paramref name="vector" /> (<typeparamref name="T" />) is not supported.</exception>
public static T GetElement<TVector, T>(this TVector vector, int index)
Copy link
Member

Choose a reason for hiding this comment

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

The indexer can't be part of the interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have a pattern of making various APIs extensions rather than instance methods for efficiency purposes (instance methods take TVector by ref, which can make the IL and other code to be larger and more complex. That in turn causes the JIT to do more work and can mess up some optimizations.

Having the extension then allows you to access the API like an instance method, without forcing the operand be address taken, without forcing the introduction of additional locals, and so on.

where TVector : ISimdVector<TVector, T>
{
return TVector.GetElement(vector, index);
}

#pragma warning disable CS8500 // This takes the address of, gets the size of, or declares a pointer to a managed type ('T')
/// <summary>Stores a vector at the given destination.</summary>
/// <typeparam name="TVector">The type of the vector.</typeparam>
/// <typeparam name="T">The type of the elements in the vector.</typeparam>
/// <param name="source">The vector that will be stored.</param>
/// <param name="destination">The destination at which <paramref name="source" /> will be stored.</param>
/// <exception cref="NotSupportedException">The type of <paramref name="source" /> (<typeparamref name="T" />) is not supported.</exception>
public static void Store<TVector, T>(this TVector source, T* destination)
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
where TVector : ISimdVector<TVector, T>
{
TVector.Store(source, destination);
}

/// <summary>Stores a vector at the given aligned destination.</summary>
/// <typeparam name="TVector">The type of the vector.</typeparam>
/// <typeparam name="T">The type of the elements in the vector.</typeparam>
/// <param name="source">The vector that will be stored.</param>
/// <param name="destination">The aligned destination at which <paramref name="source" /> will be stored.</param>
/// <exception cref="NotSupportedException">The type of <paramref name="source" /> (<typeparamref name="T" />) is not supported.</exception>
public static void StoreAligned<TVector, T>(this TVector source, T* destination)
where TVector : ISimdVector<TVector, T>
{
TVector.StoreAligned(source, destination);
}

/// <summary>Stores a vector at the given aligned destination.</summary>
/// <typeparam name="TVector">The type of the vector.</typeparam>
/// <typeparam name="T">The type of the elements in the vector.</typeparam>
/// <param name="source">The vector that will be stored.</param>
/// <param name="destination">The aligned destination at which <paramref name="source" /> will be stored.</param>
/// <remarks>This method may bypass the cache on certain platforms.</remarks>
/// <exception cref="NotSupportedException">The type of <paramref name="source" /> (<typeparamref name="T" />) is not supported.</exception>
public static void StoreAlignedNonTemporal<TVector, T>(this TVector source, T* destination)
where TVector : ISimdVector<TVector, T>
{
TVector.StoreAlignedNonTemporal(source, destination);
}
#pragma warning restore CS8500 // This takes the address of, gets the size of, or declares a pointer to a managed type ('T')

/// <summary>Stores a vector at the given destination.</summary>
/// <typeparam name="TVector">The type of the vector.</typeparam>
/// <typeparam name="T">The type of the elements in the vector.</typeparam>
/// <param name="vector">The vector that will be stored.</param>
/// <param name="destination">The destination at which the vector will be stored.</param>
/// <exception cref="NotSupportedException">The type of the elements in the vector (<typeparamref name="T" />) is not supported.</exception>
public static void StoreUnsafe<TVector, T>(this TVector vector, ref T destination)
where TVector : ISimdVector<TVector, T>
{
TVector.StoreUnsafe(vector, ref destination);
}

/// <summary>Stores a vector at the given destination.</summary>
/// <typeparam name="TVector">The type of the vector.</typeparam>
/// <typeparam name="T">The type of the elements in the vector.</typeparam>
/// <param name="vector">The vector that will be stored.</param>
/// <param name="destination">The destination to which <paramref name="elementOffset" /> will be added before the vector will be stored.</param>
/// <param name="elementOffset">The element offset from <paramref name="destination" /> from which the vector will be stored.</param>
/// <exception cref="NotSupportedException">The type of the elements in the vector (<typeparamref name="T" />) is not supported.</exception>
public static void StoreUnsafe<TVector, T>(this TVector vector, ref T destination, nuint elementOffset)
where TVector : ISimdVector<TVector, T>
{
TVector.StoreUnsafe(vector, ref destination, elementOffset);
}

/// <summary>Converts the given vector to a scalar containing the value of the first element.</summary>
/// <typeparam name="TVector">The type of the vector.</typeparam>
/// <typeparam name="T">The type of the elements in the vector.</typeparam>
/// <param name="vector">The vector to get the first element from.</param>
/// <returns>A scalar <typeparamref name="T" /> containing the value of the first element.</returns>
/// <exception cref="NotSupportedException">The type of the elements in the vector (<typeparamref name="T" />) is not supported.</exception>
public static T ToScalar<TVector, T>(this TVector vector)
where TVector : ISimdVector<TVector, T>
{
return TVector.ToScalar(vector);
}

/// <summary>Tries to copy a vector to a given span.</summary>
/// <typeparam name="TVector">The type of the vector.</typeparam>
/// <typeparam name="T">The type of the elements in the vector.</typeparam>
/// <param name="vector">The vector to copy.</param>
/// <param name="destination">The span to which <paramref name="destination" /> is copied.</param>
/// <returns><c>true</c> if <paramref name="vector" /> was successfully copied to <paramref name="destination" />; otherwise, <c>false</c> if the length of <paramref name="destination" /> is less than <see cref="ISimdVector{TVector, T}.Count" />.</returns>
/// <exception cref="NotSupportedException">The type of the elements in the vector (<typeparamref name="T" />) is not supported.</exception>
public static bool TryCopyTo<TVector, T>(this TVector vector, Span<T> destination)
where TVector : ISimdVector<TVector, T>
{
return TVector.TryCopyTo(vector, destination);
}

/// <summary>Creates a new vector with the element at the specified index set to the specified value and the remaining elements set to the same value as that in the given vector.</summary>
/// <typeparam name="TVector">The type of the vector.</typeparam>
/// <typeparam name="T">The type of the elements in the vector.</typeparam>
/// <param name="vector">The vector to get the remaining elements from.</param>
/// <param name="index">The index of the element to set.</param>
/// <param name="value">The value to set the element to.</param>
/// <returns>A vector with the value of the element at <paramref name="index" /> set to <paramref name="value" /> and the remaining elements set to the same value as that in <paramref name="vector" />.</returns>
/// <exception cref="ArgumentOutOfRangeException"><paramref name="index" /> was less than zero or greater than the number of elements.</exception>
/// <exception cref="NotSupportedException">The type of the elements in the vector (<typeparamref name="T" />) is not supported.</exception>
public static TVector WithElement<TVector, T>(this TVector vector, int index, T value)
where TVector : ISimdVector<TVector, T>
{
return TVector.WithElement(vector, index, value);
}
}
}
Loading
Loading