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

API proposal: Enable AsVector<T> in Vector<T> #508

Closed
msedi opened this issue Dec 4, 2019 · 10 comments · Fixed by #47150
Closed

API proposal: Enable AsVector<T> in Vector<T> #508

msedi opened this issue Dec 4, 2019 · 10 comments · Fixed by #47150
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics Cost:S Work that requires one engineer up to 1 week help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@msedi
Copy link

msedi commented Dec 4, 2019

Vector<T> is a helpful tool to improve performance of certain code parts without large efforts.
For number crunching we are using the Vector<T> very often, since our data is pretty large and the application of Vector<T> really boosts performance. Also when it comes to data conversion or data reinterpretation Vector<T> is an essential part and has some methods to perform also these tasks.

Rationale and Usage

However, the AsVector"T" in System.Numerics.Vector can only be used comfortably when explicitly using their names for the given data type. The problem is that when dealing with a "generic" environment where the datatypes are not known in advance and can change often it is currently hard to use the above method. The same holds true for the 'ConvertTo"T"' routines that could als be expressed with the API suggestion below.

Following example illustrates the problem:

// Cast the values from byte to "arbitrary" type.
// Assume the length of bytes are an integer multiple of data type size of the result.
public unsafe static T[] ReadArray<T>(byte[] data) where T : unmanaged
{
    int Lsource = Vector<byte>.Count;
    int Ltarget = Vector<T>.Count;

    int N = data.Length % data.Length;
    T[] result = new T[data.Length / sizeof(T)];
    int isource = 0;
    int itarget = 0;

    for (; isource < N; isource += Lsource, itarget += Ltarget)
    {
        if (typeof(T) == typeof(byte))
            Vector.AsVectorByte(new Vector<byte>(data, isource)).CopyTo((byte[])(object)result, itarget);
        else if (typeof(T) == typeof(sbyte))
            Vector.AsVectorSByte(new Vector<byte>(data, isource)).CopyTo((sbyte[])(object)result, itarget);
        else if (typeof(T) == typeof(short))
            Vector.AsVectorInt16(new Vector<byte>(data, isource)).CopyTo((short[])(object)result, itarget);
        else if (typeof(T) == typeof(ushort))
            Vector.AsVectorUInt16(new Vector<byte>(data, isource)).CopyTo((ushort[])(object)result, itarget);
        else if (typeof(T) == typeof(int))
            Vector.AsVectorInt32(new Vector<byte>(data, isource)).CopyTo((int[])(object)result, itarget);
        else if (typeof(T) == typeof(uint))
            Vector.AsVectorUInt32(new Vector<byte>(data, isource)).CopyTo((uint[])(object)result, itarget);
        else if (typeof(T) == typeof(long))
            Vector.AsVectorInt64(new Vector<byte>(data, isource)).CopyTo((long[])(object)result, itarget);
        else if (typeof(T) == typeof(ulong))
            Vector.AsVectorUInt64(new Vector<byte>(data, isource)).CopyTo((ulong[])(object)result, itarget);
        else if (typeof(T) == typeof(float))
            Vector.AsVectorSingle(new Vector<byte>(data, isource)).CopyTo((float[])(object)result, itarget);
        else if (typeof(T) == typeof(double))
            Vector.AsVectorDouble(new Vector<byte>(data, isource)).CopyTo((double[])(object)result, itarget);
        else
            throw new NotSupportedException();
    }

    return result;
}

Proposed API

The proposed API for this specific case is only a hand-crafted if..else solution as a wrapper around the current interface, but would solve problems in methods where the input and output datatypes can be better expressed by using generics. Following method is an API proposal that solves the problem for generic environments.

namespace System.Numerics
{
    public partial class Vector
    {
        public static Vector<TTo> As<TFrom, TTo>(Vector<TFrom> vector)
            where TFrom : struct
            where TTo : struct;
    }
}

Original Proposal

AsVector

public unsafe static Vector<Tout> AsVector<Tin, Tout>(Tin[] data) where Tin : struct where Tout : struct
{
    if (typeof(Tout) == typeof(byte))
        return (Vector<Tout>)(object)Vector.AsVectorByte(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(sbyte))
        return (Vector<Tout>)(object)Vector.AsVectorSByte(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(Tin))
        return (Vector<Tout>)(object)Vector.AsVectorInt16(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(ushort))
        return (Vector<Tout>)(object)Vector.AsVectorUInt16(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(int))
        return (Vector<Tout>)(object)Vector.AsVectorInt32(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(uint))
        return (Vector<Tout>)(object)Vector.AsVectorUInt32(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(long))
        return (Vector<Tout>)(object)Vector.AsVectorInt64(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(ulong))
        return (Vector<Tout>)(object)Vector.AsVectorUInt64(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(float))
        return (Vector<Tout>)(object)Vector.AsVectorSingle(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(double))
        return (Vector<Tout>)(object)Vector.AsVectorDouble(new Vector<Tin>(data));
    else
        throw new NotSupportedException();
}

ConvertTo

When writing the proposal for ´ConvertTo` I encountered a problem where ConvertTo only supports a certain range of data types. I assume that this is due the support of SSE only. The question here is if it can be extended to all datatypes or at least have some sort of fallback that can deal with other datatypes.

public unsafe static Vector<Tout> ConvertTo<Tin, Tout>(Tin[] data) where Tin : struct where Tout : struct
{
    if (typeof(Tout) == typeof(byte))
        return (Vector<Tout>)(object)Vector.ConvertToByte(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(sbyte))
        return (Vector<Tout>)(object)Vector.ConvertToSByte(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(Tin))
        return (Vector<Tout>)(object)Vector.ConvertToInt16(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(ushort))
        return (Vector<Tout>)(object)Vector.AsVectorUInt16(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(int))
        return (Vector<Tout>)(object)Vector.AsVectorInt32(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(uint))
        return (Vector<Tout>)(object)Vector.AsVectorUInt32(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(long))
        return (Vector<Tout>)(object)Vector.AsVectorInt64(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(ulong))
        return (Vector<Tout>)(object)Vector.ConvertToUInt64(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(float))
        return (Vector<Tout>)(object)Vector.ConvertToSingle(new Vector<Tin>(data));
    else if (typeof(Tout) == typeof(double))
        return (Vector<Tout>)(object)Vector.ConvertToDouble(new Vector<Tin>(data));
    else
        throw new NotSupportedException();
}
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Numerics untriaged New issue has not been triaged by the area owner labels Dec 4, 2019
@GrabYourPitchforks GrabYourPitchforks added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 4, 2019
@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Feb 10, 2020
@tannergooding
Copy link
Member

@msedi, I think this is a fine proposal to take to API review. However, could you please update the original post to explicitly call out the API you would like to be exposed?

Our API review process is detailed here: https://github.com/dotnet/runtime/blob/master/docs/project/api-review-process.md
A "good example" of an API proposal is listed under step 1 (noting that you don't need all the sections, but we do appreciate Rationale and Proposed API at a minimum, as it helps the review process).

@msedi
Copy link
Author

msedi commented Mar 2, 2020

@tannergooding I have updated my proposal and hope that it fits your needs. Otherwise please just come back and hit me ;-)

@tannergooding
Copy link
Member

For AsVector, could you use TFrom/TTo. These match the names we use in some of the other conversion APIs (such as Unsafe.As).

We don't need the implementation listed as that won't be important for API review. Instead, you could probably just comment that TFrom/TTo are valid for any T currently supported by Vector<T>.
The actual implementation, if approved, will likely be specialized in the JIT and have a software fallback which calls Unsafe.As (similar to https://source.dot.net/#System.Private.CoreLib/Vector128.cs,1b217acac2b14f12)

As for ConvertTo, that should likely be a separate proposal. The reason it doesn't exist today is because there are additional considerations needed. Vector<byte> could contain 16/32 elements while Vector<float> only contains 4/8 elements, so you can't easily define a generic mechanism for conversion.

@tannergooding tannergooding added this to the Future milestone Jun 23, 2020
@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 6, 2020
@tannergooding
Copy link
Member

I updated the original post to just call out the System.Numerics.Vector.As<TFrom, TTo> API as proposed (without any impl code). The original proposal was moved under a new section.

As per my last comment, the ConvertTo method should be moved into its own proposal.

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Oct 6, 2020
@terrajobst
Copy link
Member

terrajobst commented Oct 6, 2020

Video

  • Looks good as proposed, unless @CarolEidt has concerns
namespace System.Numerics
{
    public partial class Vector
    {
        public static Vector<TTo> As<TFrom, TTo>(Vector<TFrom> vector)
            where TFrom : struct
            where TTo : struct;
    }
}

@CarolEidt
Copy link
Contributor

No concerns here

@tannergooding tannergooding added the help wanted [up-for-grabs] Good issue for external contributors label Oct 6, 2020
@tannergooding tannergooding added the Cost:S Work that requires one engineer up to 1 week label Jan 15, 2021
@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-approved API was approved in API review, it can be implemented labels Jan 19, 2021
@tannergooding
Copy link
Member

Marking this as ready-for-review as it likely should have been an extension method to mirror the equivalent methods on Vector128 and Vector256

public partial class Vector
{
    public static Vector<TTo> As<TFrom, TTo>(this Vector<TFrom> vector)
        where TFrom : struct
        where TTo : struct;
}

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 20, 2021
@huoyaoyuan
Copy link
Member

Since we don't have partial type inference now, the extension method still needs to be used as vector.As<TFrom, TTo>.

What about an instance method that delegates to the static cast?

@tannergooding
Copy link
Member

What about an instance method that delegates to the static cast?

The reason why most of the methods were made extensions for Vector128 and Vector256 was to ensure good codegen and avoid the structs from being reference taken.
When we reviewed the Vector128/Vector256 variants having too do .As<byte, int> was deemed an acceptable drawback, particularly when it should be limited to (realistically) no more than once per input and once per return on fully generic methods.

I'd expect the same would be true for this variant.

@terrajobst
Copy link
Member

terrajobst commented Jan 21, 2021

Video

  • Looks good as proposed
public partial class Vector
{
    public static Vector<TTo> As<TFrom, TTo>(this Vector<TFrom> vector)
        where TFrom : struct
        where TTo : struct;
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 21, 2021
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Feb 1, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 3, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 3, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics Cost:S Work that requires one engineer up to 1 week help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants