Proposals for Intel HW intrinsic API changes for vNext

[4creators]

Writing real code with already implemented HW intrinsics provides very good tests for their usability and quality of design. We start to create implementations which immediately provide direct test of quality of available API surface. I would like to propose to discuss issues and possible change proposals using this issue as an umbrella.

[4creators]

cc @CarolEidt @eerhardt @fiigii @jkotas @sdmaclea @tannergooding

[4creators]

Current experience with writing very simple Sse and Sse2 code points to one pain point StaticCast design. Due to using two type arguments it requires writing explicitly all generic type arguments and the function name could be shortened.

My proposal is to change API as follows:

// Instead of:
        public static Vector128<U> StaticCast<T, U>(Vector128<T> value) where T : struct where U : struct

// Use the following definitions:
        public static Vector128<T> Cast<T>(Vector128<double> value) where T : struct
        public static Vector128<T> Cast<T>(Vector128<float> value) where T : struct
        public static Vector128<T> Cast<T>(Vector128<long> value) where T : struct
        public static Vector128<T> Cast<T>(Vector128<ulong> value) where T : struct
        public static Vector128<T> Cast<T>(Vector128<int> value) where T : struct
        public static Vector128<T> Cast<T>(Vector128<uint> value) where T : struct
        public static Vector128<T> Cast<T>(Vector128<short> value) where T : struct
        public static Vector128<T> Cast<T>(Vector128<ushort> value) where T : struct
        public static Vector128<T> Cast<T>(Vector128<byte> value) where T : struct
        public static Vector128<T> Cast<T>(Vector128<sbyte> value) where T : struct

The change allows for generic type argument inference and as a result it is not necessary to use any type arguments in most situation.

For example instead of writing:

        /// <summary>
        /// __m128i _mm_set1_epi8 (char a)
        ///   HELPER
        /// </summary>
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static Vector128<byte> SetAllVector128(byte value)
        {
            Vector128<byte> vector1 = Sse.StaticCast<uint, byte>(ConvertScalarToVector128UInt32(value));
            Vector128<ushort> tmpVector1 = Sse.StaticCast<byte, ushort>(UnpackLow(vector1, vector1));
            Vector128<uint> tmpVector2 = Sse.StaticCast<ushort, uint>(UnpackLow(tmpVector1, tmpVector1));
            return Sse.StaticCast<uint, byte>(Shuffle(tmpVector2, 0));
        }

we could write:

        /// <summary>
        /// __m128i _mm_set1_epi8 (char a)
        ///   HELPER
        /// </summary>
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static Vector128<byte> SetAllVector128(byte value)
        {
            Vector128<byte> vector1 = Cast(ConvertScalarToVector128UInt32(value));
            Vector128<ushort> tmpVector1 = Cast(UnpackLow(vector1, vector1));
            Vector128<uint> tmpVector2 = Cast(UnpackLow(tmpVector1, tmpVector1));
            return Cast(Shuffle(tmpVector2, 0));
        }

And it is possible to use var locals definitions with single type argument:

        /// <summary>
        /// __m128i _mm_set1_epi8 (char a)
        ///   HELPER
        /// </summary>
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static Vector128<byte> SetAllVector128(byte value)
        {
            var vector1 = Cast<byte>(ConvertScalarToVector128UInt32(value));
            var tmpVector1 = Cast<ushort>(UnpackLow(vector1, vector1));
            var tmpVector2 = Cast<uint>(UnpackLow(tmpVector1, tmpVector1));
            return Cast(Shuffle(tmpVector2, 0));
        }

This change is very easy to implement and perhaps we could do this before we ship v2.1

@CarolEidt @eerhardt @fiigii @jkotas @sdmaclea @tannergooding

[4creators]

@CarolEidt @eerhardt @fiigii @tannergooding

Ping. It is quite serious issue for devs who would be writing code using HW intrinsics and I would strongly recommend changing API before v2.1 ships. Fix is easy and fast.

[fiigii]

Due to using two type arguments it requires writing explicitly all generic type arguments and the function name could be shortened.

Personally, I prefer to always write explicit TFrom and TTo for this kind of type-cast functions.

[4creators]

Personally, I prefer to always write explicit TFrom and TTo for this kind of type-cast functions.

But it is not the way C# code is written by majority of coders. Most of the C# language features introduced over last years are aimed at reducing amount of code written and not at increasing it.

[sdmaclea]

It is also not technically impossible to support Cast<T> and StaticCast<T,U>. Although long term I suspect API design will want to select only one.

I would also like an opinion from @CarolEidt and/or @eerhardt since this is one of the next API to add to arm64.

[CarolEidt]

This seems like a reasonable proposal - i.e. the notion of having a cast with a single generic argument. However, we are already well past the point of finalizing 2.1 API changes, and really this kind of feedback is what we are looking to get before finalizing the API.
Furthermore, I think it's worth discussing whether the simple name Cast is sufficiently expressive, as Cast (vs. StaticCast) usually implies a possible representational change.

[4creators]

we are already well past the point of finalizing 2.1 API changes

I understand it but while writing just couple of functions it was just striking how often that operation is used and how tedious it is to write it. That kind of experience is difficult or impossible to predict without writing real world code what became possible in just last couple of days.

If it is too late to change whole API perhaps we can implement Cast<T> as a software wrapper for StaticCast<T,U> as we did with SetVector128 and SetAllVector128.

[sdmaclea]

@jkotas had suggested that StaticCast<> was inconsistent with Span which chose Cast if I remember correctly. Being from C++ world, I preferred ReinterpretCast<> because both static_cast and cast in C++ imply type conversion. However that was vetoed.

[fiigii]

@jkotas had suggested that StaticCast<> was inconsistent with Span which chose Cast if I remember correctly.

Yes, I like this Cast<TFrom, TTo>.

[mikedn]

However that was vetoed.

That's unfortunate. Reinterpret is a much more suitable name for this kind of stuff. No idea why would anyone veto that.

[4creators]

I like this Cast<TFrom, TTo>.

We may have both: Cast<TFrom, TTo> and Cast<TTo>

[4creators]

With the latter implemented as software wrapper of the Cast<TFrom, TTo>

[4creators]

Reinterpret is a much more suitable name for this kind of stuff. No idea why would anyone veto that.

It is due to different naming conventions and history of C++ vs. C#

[sdmaclea]

That's unfortunate.

@mikedn I just read the original discussion in #24771 vetoed was a little strong.

[mikedn]

It is due to different naming conventions and history of C++ vs. C#

Naming conventions and history have nothing to do with this, they don't impose how to name a feature that's practically new to .NET/C#.

[4creators]

There is an interesting C# language proposal dotnet/csharplang#1349 Champion: "Partial Type Inference" which would allow to simplify usage of API with 2 type parameters.

[eerhardt]

However, we are already well past the point of finalizing 2.1 API changes, and really this kind of feedback is what we are looking to get before finalizing the API.

Agreed with this. We have to stop churning the API at some point so it can be shipped. We can change the API post-2.1, which is why we made it experimental for 2.1. This suggestion can go on that list.

[4creators]

https://github.com/dotnet/coreclr/issues/18910#issuecomment-404965603

[saucecontrol]

Does this need to be converted to a proper API proposal issue to move forward, or is it already on a review list somewhere?

[tannergooding]

I have most of these on a review list for the next x86 HWIntrinsics review session.

[saucecontrol]

Looks like the specific issues here were addressed with the As methods. @4creators can this be closed?

[4creators]

@saucecontrol Yes, it's true. Closing.