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 various new Create and conversion APIs for the Vector types #103462

Merged
merged 6 commits into from
Jun 15, 2024

Conversation

tannergooding
Copy link
Member

This resolves #92299 and #94384

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, 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.

1 similar comment
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, 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.

@@ -115,7 +64,7 @@ SIMD_AS_HWINTRINSIC_ID(VectorT, ConvertToUInt32,
SIMD_AS_HWINTRINSIC_ID(VectorT, ConvertToUInt32Native, 1, {NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_VectorT_ConvertToUInt32Native, NI_Illegal}, SimdAsHWIntrinsicFlag::None)
SIMD_AS_HWINTRINSIC_ID(VectorT, ConvertToUInt64, 1, {NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_VectorT_ConvertToUInt64}, SimdAsHWIntrinsicFlag::None)
SIMD_AS_HWINTRINSIC_ID(VectorT, ConvertToUInt64Native, 1, {NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_VectorT_ConvertToUInt64Native}, SimdAsHWIntrinsicFlag::None)
SIMD_AS_HWINTRINSIC_NM(VectorT, CreateBroadcast, ".ctor", 2, {NI_VectorT_CreateBroadcast, NI_VectorT_CreateBroadcast, NI_VectorT_CreateBroadcast, NI_VectorT_CreateBroadcast, NI_VectorT_CreateBroadcast, NI_VectorT_CreateBroadcast, NI_VectorT_CreateBroadcast, NI_VectorT_CreateBroadcast, NI_VectorT_CreateBroadcast, NI_VectorT_CreateBroadcast}, SimdAsHWIntrinsicFlag::InstanceMethod | SimdAsHWIntrinsicFlag::SpillSideEffectsOp1)
Copy link
Member Author

Choose a reason for hiding this comment

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

With this PR, the only remaining simdashwintrinsic importation handling is for Vector<T>.

After this PR is merged, we can take advantage of that fact and change the importation logic a bit. Namely, since we've moved towards having parity across Vector<T> and Vector64/128/256/512<T> we can special case Vector<T> to import as if it were Vector128/256/512<T> instead (based on its size), allowing us to remove simdashwintrinsic in its entirety and just defer down a single path.

@martenf
Copy link
Contributor

martenf commented Jun 14, 2024

Will
public static Vector512<T> Create<T>(Vector128<T> value) => Create(Vector256.Create(value, value));
and
public static Vector512<T> Create<T>(Vector256<T> value) => Create(value, value);

use _mm512_set4_ for the appropriate types on AVX512 systems or classical broadcasts?

@tannergooding
Copy link
Member Author

use mm512_set4 for the appropriate types on AVX512 systems or classical broadcasts?

Not in this PR, but I will be getting it updated in a separate PR. There is some additional work required to get the different overloads of Create to be imported and optimized correctly; so this PR mostly focuses on exposing the newly approved APIs without regressing the current codegen.

@tannergooding
Copy link
Member Author

CC. @dotnet/jit-contrib, this is ready for review. Removes the rest of the Vector2/3 handling from simdashwintrinsic and adds some of the new xplat APIs that were approved.

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

Nice! Glad that Vector<T> now has Create

Also, e.g. in #86220 @xoofx had to do AsVector128().AsVector3() for V4 to V3 conversion, now there is AsVector3 for it.

@tannergooding tannergooding merged commit f69972f into dotnet:main Jun 15, 2024
171 of 173 checks passed
@tannergooding tannergooding deleted the fix-92299-and-94384 branch June 15, 2024 13:49
@LoopedBard3
Copy link
Member

LoopedBard3 commented Jun 18, 2024

@tannergooding
Copy link
Member Author

The "regression" here is because we aren't constant folding the call to SinCos. It's an improvement for the real world scenario

@lewing
Copy link
Member

lewing commented Jun 19, 2024

mono wasm interpreter regressions here dotnet/perf-autofiling-issues#36484

cc @kg @BrzVlad

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.

[API Proposal]: VectorNNN.Create with broadcasting
5 participants