Runtime: Proposals for Intel HW intrinsic API changes for vNext

Created on 9 Mar 2018 · 23Comments · Source: dotnet/runtime

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.

area-System.Runtime.Intrinsics question

Source

4creators

Most helpful comment

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.

eerhardt on 16 Mar 2018

👍3

All 23 comments

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

4creators on 9 Mar 2018

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:

```C#
// Instead of:
public static Vector128 StaticCast(Vector128 value) where T : struct where U : struct

// Use the following definitions:
public static Vector128 Cast(Vector128 value) where T : struct
public static Vector128 Cast(Vector128 value) where T : struct
public static Vector128 Cast(Vector128 value) where T : struct
public static Vector128 Cast(Vector128 value) where T : struct
public static Vector128 Cast(Vector128 value) where T : struct
public static Vector128 Cast(Vector128 value) where T : struct
public static Vector128 Cast(Vector128 value) where T : struct
public static Vector128 Cast(Vector128 value) where T : struct
public static Vector128 Cast(Vector128 value) where T : struct
public static Vector128 Cast(Vector128 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:

```C#
        /// <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:
```C#
///

/// __m128i _mm_set1_epi8 (char a)
/// HELPER
///

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static Vector128 SetAllVector128(byte value)
{
Vector128 vector1 = Cast(ConvertScalarToVector128UInt32(value));
Vector128 tmpVector1 = Cast(UnpackLow(vector1, vector1));
Vector128 tmpVector2 = Cast(UnpackLow(tmpVector1, tmpVector1));
return Cast(Shuffle(tmpVector2, 0));
}


And it is possible to use `var` locals definitions with single type argument:
```C#
        /// <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 on 13 Mar 2018

❤2 👍1

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

4creators on 15 Mar 2018

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.

fiigii on 15 Mar 2018

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.

4creators on 15 Mar 2018

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.

sdmaclea on 15 Mar 2018

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.

CarolEidt on 15 Mar 2018

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.

4creators on 15 Mar 2018

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

sdmaclea on 15 Mar 2018

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

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

fiigii on 15 Mar 2018

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.

mikedn on 15 Mar 2018

I like this Cast<TFrom, TTo>.

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

4creators on 15 Mar 2018

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

4creators on 15 Mar 2018

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#

4creators on 15 Mar 2018

That's unfortunate.

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

sdmaclea on 15 Mar 2018

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

mikedn on 15 Mar 2018

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

4creators on 15 Mar 2018

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.

eerhardt on 16 Mar 2018

👍3

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

4creators on 18 Jul 2018

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