Minor cleanup of the Vector64/128/256/512 implementations to improve fallbacks by tannergooding · Pull Request #103095 · dotnet/runtime

tannergooding · 2024-06-05T21:52:54Z

Continuation of #102301, just cleaning up some of the logic to require less inlining and defer to the same size core API for the fallback implementation where trivially possible.

…fallbacks

tannergooding · 2024-06-05T21:58:37Z

- Vector256.Divide(left._upper, right._upper)
- );
- }
+ public static Vector512<T> Divide<T>(Vector512<T> left, Vector512<T> right) => left / right;


Cases like this were where size regressions were coming from. This ensures that instead we hit the intrinsic path when we're accelerated and otherwise we get the same overall codegen as before.

tannergooding · 2024-06-05T21:59:59Z

 /// <exception cref="NotSupportedException">The type of <paramref name="left" /> and <paramref name="right" /> (<typeparamref name="T" />) is not supported.</exception>
 [Intrinsic]
- public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => left * right;
+ public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => right * left;


Cases like this aren't strictly needed, but it avoids needing +1 inlining step when we aren't accelerated and defers to the core algorithm instead.
This overload is just convenience so that scalar * vector and vector * scalar both work, since the operation is commutative, so there's no optimization opportunity missing here either.

dotnet-policy-service · 2024-06-05T23:26:13Z

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

tannergooding · 2024-06-06T11:56:17Z

@MihuBot

tannergooding · 2024-06-06T18:12:06Z

Size regressions in APIs like ISimdVector.Divide is because we're now emitting the actual div instruction instead of a call to Scalar<T>.Div.

For Sum, it's because the implementation was made to be correct. It was buggy for floating-point before and was non-deterministic between direct and indirect invocations. This in turn causes extra instructions to be emitted in a number of APIs.

There's a few places where the fallbacks involving AndNot are worse because the JIT gives up on recognizing x & ~y in this particular case (it currently gives up for reverse ops or any potential side effect flag).

The meaningful places that will execute when IsHardwareAccelerated == true have improved as expected

tannergooding · 2024-06-06T18:12:13Z

CC. @EgorBo

EgorBo

LGTM

tannergooding · 2024-06-05T21:52:54Z

Continuation of #102301, just cleaning up some of the logic to require less inlining and defer to the same size core API for the fallback implementation where trivially possible.

…fallbacks

tannergooding · 2024-06-05T21:58:37Z

- Vector256.Divide(left._upper, right._upper)
- );
- }
+ public static Vector512<T> Divide<T>(Vector512<T> left, Vector512<T> right) => left / right;


Cases like this were where size regressions were coming from. This ensures that instead we hit the intrinsic path when we're accelerated and otherwise we get the same overall codegen as before.

tannergooding · 2024-06-05T21:59:59Z

 /// <exception cref="NotSupportedException">The type of <paramref name="left" /> and <paramref name="right" /> (<typeparamref name="T" />) is not supported.</exception>
 [Intrinsic]
- public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => left * right;
+ public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => right * left;


Cases like this aren't strictly needed, but it avoids needing +1 inlining step when we aren't accelerated and defers to the core algorithm instead.
This overload is just convenience so that scalar * vector and vector * scalar both work, since the operation is commutative, so there's no optimization opportunity missing here either.

dotnet-policy-service · 2024-06-05T23:26:13Z

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

tannergooding · 2024-06-06T11:56:17Z

@MihuBot

tannergooding · 2024-06-06T18:12:06Z

Size regressions in APIs like ISimdVector.Divide is because we're now emitting the actual div instruction instead of a call to Scalar<T>.Div.

For Sum, it's because the implementation was made to be correct. It was buggy for floating-point before and was non-deterministic between direct and indirect invocations. This in turn causes extra instructions to be emitted in a number of APIs.

There's a few places where the fallbacks involving AndNot are worse because the JIT gives up on recognizing x & ~y in this particular case (it currently gives up for reverse ops or any potential side effect flag).

The meaningful places that will execute when IsHardwareAccelerated == true have improved as expected

tannergooding · 2024-06-06T18:12:13Z

CC. @EgorBo

EgorBo

LGTM

tannergooding · 2024-06-05T21:52:54Z

Continuation of #102301, just cleaning up some of the logic to require less inlining and defer to the same size core API for the fallback implementation where trivially possible.

…fallbacks

tannergooding · 2024-06-05T21:58:37Z

- Vector256.Divide(left._upper, right._upper)
- );
- }
+ public static Vector512<T> Divide<T>(Vector512<T> left, Vector512<T> right) => left / right;


Cases like this were where size regressions were coming from. This ensures that instead we hit the intrinsic path when we're accelerated and otherwise we get the same overall codegen as before.

tannergooding · 2024-06-05T21:59:59Z

 /// <exception cref="NotSupportedException">The type of <paramref name="left" /> and <paramref name="right" /> (<typeparamref name="T" />) is not supported.</exception>
 [Intrinsic]
- public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => left * right;
+ public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => right * left;


Cases like this aren't strictly needed, but it avoids needing +1 inlining step when we aren't accelerated and defers to the core algorithm instead.
This overload is just convenience so that scalar * vector and vector * scalar both work, since the operation is commutative, so there's no optimization opportunity missing here either.

dotnet-policy-service · 2024-06-05T23:26:13Z

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

tannergooding · 2024-06-06T11:56:17Z

@MihuBot

tannergooding · 2024-06-06T18:12:06Z

Size regressions in APIs like ISimdVector.Divide is because we're now emitting the actual div instruction instead of a call to Scalar<T>.Div.

For Sum, it's because the implementation was made to be correct. It was buggy for floating-point before and was non-deterministic between direct and indirect invocations. This in turn causes extra instructions to be emitted in a number of APIs.

There's a few places where the fallbacks involving AndNot are worse because the JIT gives up on recognizing x & ~y in this particular case (it currently gives up for reverse ops or any potential side effect flag).

The meaningful places that will execute when IsHardwareAccelerated == true have improved as expected

tannergooding · 2024-06-06T18:12:13Z

CC. @EgorBo

EgorBo

LGTM

tannergooding · 2024-06-05T21:52:54Z

Continuation of #102301, just cleaning up some of the logic to require less inlining and defer to the same size core API for the fallback implementation where trivially possible.

…fallbacks

tannergooding · 2024-06-05T21:58:37Z

- Vector256.Divide(left._upper, right._upper)
- );
- }
+ public static Vector512<T> Divide<T>(Vector512<T> left, Vector512<T> right) => left / right;


Cases like this were where size regressions were coming from. This ensures that instead we hit the intrinsic path when we're accelerated and otherwise we get the same overall codegen as before.

tannergooding · 2024-06-05T21:59:59Z

 /// <exception cref="NotSupportedException">The type of <paramref name="left" /> and <paramref name="right" /> (<typeparamref name="T" />) is not supported.</exception>
 [Intrinsic]
- public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => left * right;
+ public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => right * left;


Cases like this aren't strictly needed, but it avoids needing +1 inlining step when we aren't accelerated and defers to the core algorithm instead.
This overload is just convenience so that scalar * vector and vector * scalar both work, since the operation is commutative, so there's no optimization opportunity missing here either.

dotnet-policy-service · 2024-06-05T23:26:13Z

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

tannergooding · 2024-06-06T11:56:17Z

@MihuBot

tannergooding · 2024-06-06T18:12:06Z

Size regressions in APIs like ISimdVector.Divide is because we're now emitting the actual div instruction instead of a call to Scalar<T>.Div.

For Sum, it's because the implementation was made to be correct. It was buggy for floating-point before and was non-deterministic between direct and indirect invocations. This in turn causes extra instructions to be emitted in a number of APIs.

There's a few places where the fallbacks involving AndNot are worse because the JIT gives up on recognizing x & ~y in this particular case (it currently gives up for reverse ops or any potential side effect flag).

The meaningful places that will execute when IsHardwareAccelerated == true have improved as expected

tannergooding · 2024-06-06T18:12:13Z

CC. @EgorBo

EgorBo

LGTM

tannergooding · 2024-06-05T21:52:54Z

Continuation of #102301, just cleaning up some of the logic to require less inlining and defer to the same size core API for the fallback implementation where trivially possible.

…fallbacks

tannergooding · 2024-06-05T21:58:37Z

- Vector256.Divide(left._upper, right._upper)
- );
- }
+ public static Vector512<T> Divide<T>(Vector512<T> left, Vector512<T> right) => left / right;


Cases like this were where size regressions were coming from. This ensures that instead we hit the intrinsic path when we're accelerated and otherwise we get the same overall codegen as before.

tannergooding · 2024-06-05T21:59:59Z

 /// <exception cref="NotSupportedException">The type of <paramref name="left" /> and <paramref name="right" /> (<typeparamref name="T" />) is not supported.</exception>
 [Intrinsic]
- public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => left * right;
+ public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => right * left;


Cases like this aren't strictly needed, but it avoids needing +1 inlining step when we aren't accelerated and defers to the core algorithm instead.
This overload is just convenience so that scalar * vector and vector * scalar both work, since the operation is commutative, so there's no optimization opportunity missing here either.

dotnet-policy-service · 2024-06-05T23:26:13Z

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

tannergooding · 2024-06-06T11:56:17Z

@MihuBot

tannergooding · 2024-06-06T18:12:06Z

Size regressions in APIs like ISimdVector.Divide is because we're now emitting the actual div instruction instead of a call to Scalar<T>.Div.

For Sum, it's because the implementation was made to be correct. It was buggy for floating-point before and was non-deterministic between direct and indirect invocations. This in turn causes extra instructions to be emitted in a number of APIs.

There's a few places where the fallbacks involving AndNot are worse because the JIT gives up on recognizing x & ~y in this particular case (it currently gives up for reverse ops or any potential side effect flag).

The meaningful places that will execute when IsHardwareAccelerated == true have improved as expected

tannergooding · 2024-06-06T18:12:13Z

CC. @EgorBo

EgorBo

LGTM

tannergooding · 2024-06-05T21:52:54Z

Continuation of #102301, just cleaning up some of the logic to require less inlining and defer to the same size core API for the fallback implementation where trivially possible.

…fallbacks

tannergooding · 2024-06-05T21:58:37Z

- Vector256.Divide(left._upper, right._upper)
- );
- }
+ public static Vector512<T> Divide<T>(Vector512<T> left, Vector512<T> right) => left / right;


Cases like this were where size regressions were coming from. This ensures that instead we hit the intrinsic path when we're accelerated and otherwise we get the same overall codegen as before.

tannergooding · 2024-06-05T21:59:59Z

 /// <exception cref="NotSupportedException">The type of <paramref name="left" /> and <paramref name="right" /> (<typeparamref name="T" />) is not supported.</exception>
 [Intrinsic]
- public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => left * right;
+ public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => right * left;


Cases like this aren't strictly needed, but it avoids needing +1 inlining step when we aren't accelerated and defers to the core algorithm instead.
This overload is just convenience so that scalar * vector and vector * scalar both work, since the operation is commutative, so there's no optimization opportunity missing here either.

dotnet-policy-service · 2024-06-05T23:26:13Z

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

tannergooding · 2024-06-06T11:56:17Z

@MihuBot

tannergooding · 2024-06-06T18:12:06Z

Size regressions in APIs like ISimdVector.Divide is because we're now emitting the actual div instruction instead of a call to Scalar<T>.Div.

For Sum, it's because the implementation was made to be correct. It was buggy for floating-point before and was non-deterministic between direct and indirect invocations. This in turn causes extra instructions to be emitted in a number of APIs.

There's a few places where the fallbacks involving AndNot are worse because the JIT gives up on recognizing x & ~y in this particular case (it currently gives up for reverse ops or any potential side effect flag).

The meaningful places that will execute when IsHardwareAccelerated == true have improved as expected

tannergooding · 2024-06-06T18:12:13Z

CC. @EgorBo

EgorBo

LGTM

tannergooding · 2024-06-05T21:52:54Z

Continuation of #102301, just cleaning up some of the logic to require less inlining and defer to the same size core API for the fallback implementation where trivially possible.

…fallbacks

tannergooding · 2024-06-05T21:58:37Z

- Vector256.Divide(left._upper, right._upper)
- );
- }
+ public static Vector512<T> Divide<T>(Vector512<T> left, Vector512<T> right) => left / right;


Cases like this were where size regressions were coming from. This ensures that instead we hit the intrinsic path when we're accelerated and otherwise we get the same overall codegen as before.

tannergooding · 2024-06-05T21:59:59Z

 /// <exception cref="NotSupportedException">The type of <paramref name="left" /> and <paramref name="right" /> (<typeparamref name="T" />) is not supported.</exception>
 [Intrinsic]
- public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => left * right;
+ public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => right * left;


Cases like this aren't strictly needed, but it avoids needing +1 inlining step when we aren't accelerated and defers to the core algorithm instead.
This overload is just convenience so that scalar * vector and vector * scalar both work, since the operation is commutative, so there's no optimization opportunity missing here either.

dotnet-policy-service · 2024-06-05T23:26:13Z

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

tannergooding · 2024-06-06T11:56:17Z

@MihuBot

tannergooding · 2024-06-06T18:12:06Z

Size regressions in APIs like ISimdVector.Divide is because we're now emitting the actual div instruction instead of a call to Scalar<T>.Div.

For Sum, it's because the implementation was made to be correct. It was buggy for floating-point before and was non-deterministic between direct and indirect invocations. This in turn causes extra instructions to be emitted in a number of APIs.

There's a few places where the fallbacks involving AndNot are worse because the JIT gives up on recognizing x & ~y in this particular case (it currently gives up for reverse ops or any potential side effect flag).

The meaningful places that will execute when IsHardwareAccelerated == true have improved as expected

tannergooding · 2024-06-06T18:12:13Z

CC. @EgorBo

EgorBo

LGTM

tannergooding · 2024-06-05T21:52:54Z

Continuation of #102301, just cleaning up some of the logic to require less inlining and defer to the same size core API for the fallback implementation where trivially possible.

…fallbacks

tannergooding · 2024-06-05T21:58:37Z

- Vector256.Divide(left._upper, right._upper)
- );
- }
+ public static Vector512<T> Divide<T>(Vector512<T> left, Vector512<T> right) => left / right;


Cases like this were where size regressions were coming from. This ensures that instead we hit the intrinsic path when we're accelerated and otherwise we get the same overall codegen as before.

tannergooding · 2024-06-05T21:59:59Z

 /// <exception cref="NotSupportedException">The type of <paramref name="left" /> and <paramref name="right" /> (<typeparamref name="T" />) is not supported.</exception>
 [Intrinsic]
- public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => left * right;
+ public static Vector512<T> Multiply<T>(T left, Vector512<T> right) => right * left;


Cases like this aren't strictly needed, but it avoids needing +1 inlining step when we aren't accelerated and defers to the core algorithm instead.
This overload is just convenience so that scalar * vector and vector * scalar both work, since the operation is commutative, so there's no optimization opportunity missing here either.

dotnet-policy-service · 2024-06-05T23:26:13Z

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

tannergooding · 2024-06-06T11:56:17Z

@MihuBot

tannergooding · 2024-06-06T18:12:06Z

Size regressions in APIs like ISimdVector.Divide is because we're now emitting the actual div instruction instead of a call to Scalar<T>.Div.

For Sum, it's because the implementation was made to be correct. It was buggy for floating-point before and was non-deterministic between direct and indirect invocations. This in turn causes extra instructions to be emitted in a number of APIs.

There's a few places where the fallbacks involving AndNot are worse because the JIT gives up on recognizing x & ~y in this particular case (it currently gives up for reverse ops or any potential side effect flag).

The meaningful places that will execute when IsHardwareAccelerated == true have improved as expected

tannergooding · 2024-06-06T18:12:13Z

CC. @EgorBo

EgorBo

LGTM

Minor cleanup of the Vector64/128/256/512 implementations to improve …
4bab1d8
…fallbacks

ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 5, 2024

dotnet-policy-serviceBot assigned tannergooding Jun 5, 2024

tannergooding commented Jun 5, 2024
View reviewed changes

tannergooding added area-System.Runtime.Intrinsics and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 5, 2024

tannergooding added 3 commits June 5, 2024 17:16

Ensure gtNewSimdSumNode maintains consistency with the software fallback
0ed78a4

Ensure Vector128.Sum also does pairwise adds for floating-point
69f352a

Use the right type in the gtNewSimdBinOpNode call
4175905

MihuBot mentioned this pull request Jun 6, 2024
[X64] [tannergooding] Minor cleanup of the Vector64/128/256/512 implementations to improve fall ... MihuBot/runtime-utils#404
Open

Don't regress fallback scenarios using AndNot
c465d2d

MihuBot mentioned this pull request Jun 6, 2024
[X64] [tannergooding] Minor cleanup of the Vector64/128/256/512 implementations to improve fall ... MihuBot/runtime-utils#405
Open

This was referenced Jun 6, 2024
Dead lettering tests #101524
Closed
[browser][MT] Tests timed out #102559
Closed

EgorBo approved these changes Jun 7, 2024
View reviewed changes

tannergooding merged commit e012fd4 into dotnet:mainJun 7, 2024

tannergooding deleted the proto-102275 branch June 7, 2024 17:36

github-actionsBot locked and limited conversation to collaborators Jul 8, 2024