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

Mark and expose additional Vector functions as Intrinsic #77562

Merged
merged 13 commits into from
Nov 2, 2022

Conversation

tannergooding
Copy link
Member

This resolves #76593

@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Oct 27, 2022
@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.

@ghost ghost assigned tannergooding Oct 27, 2022
@ghost
Copy link

ghost commented Oct 27, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #76593

Author: tannergooding
Assignees: -
Labels:

area-CodeGen-coreclr, new-api-needs-documentation

Milestone: -

@build-analysis build-analysis bot mentioned this pull request Oct 27, 2022
2 tasks
@@ -1150,7 +1151,7 @@ internal static Vector256<T> Create<T>(Vector128<T> lower, Vector128<T> upper)
/// <returns>A new <see cref="Vector256{T}" /> instance with the first element initialized to <paramref name="value" /> and the remaining elements initialized to zero.</returns>
/// <exception cref="NotSupportedException">The type of <paramref name="value" /> (<typeparamref name="T" />) is not supported.</exception>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static Vector256<T> CreateScalar<T>(T value)
public static Vector256<T> CreateScalar<T>(T value)
Copy link
Member

Choose a reason for hiding this comment

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

Could we change this to

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static Vector256<T> CreateScalar<T>(T value)
    where T : struct
{
    if (Avx.IsSupported)
    {
         return Vector128.CreateScalar(value).ToVector256Unsafe();
    }

    return Vector128.CreateScalar(value).ToVector256();
}

I end up not using this method a lot of times because it currently emits an extra vmovaps to clear the upper lane after the scalar move, even though the scalar move will have already zeroed the upper lane with VEX encoding.

The extra vmovaps should get no-op'ed by the CPU front-end anyway, but this is a common need, so smaller codegen would be better.

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'm going to update this to be a proper intrinsic in a follow up PR.

@tannergooding tannergooding marked this pull request as ready for review October 31, 2022 16:02
@tannergooding
Copy link
Member Author

CC. @dotnet/jit-contrib for the runtime side tweaks

@@ -19573,9 +19681,22 @@ GenTree* Compiler::gtNewSimdBinOpNode(genTreeOps op,
case GT_RSH:
case GT_RSZ:
{
assert(!varTypeIsFloating(simdBaseType));
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that Vector*<T> is going to support shifting so it makes sense that the restriction is lifted, then you normalize the base type.

@@ -2030,17 +2055,21 @@ GenTree* Compiler::impBaseIntrinsic(NamedIntrinsic intrinsic,

if ((simdSize != 32) || compExactlyDependsOn(InstructionSet_AVX2))
{
genTreeOps op = varTypeIsUnsigned(simdBaseType) ? GT_RSZ : GT_RSH;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the base type is unsigned, is op_RightShift basically going to be the same thing as op_UnsignedRightShift?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, that's how op_RightShift is expected to behave for unsigned types

Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

JIT side of things look good to me.

@tannergooding
Copy link
Member Author

/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm, runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vector64/128/256/512<T> and Vector<T> Consistency/Enhancements
3 participants