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

Extend Vector64<T>, Vector128<T>, and Vector256<T> to support nint and nuint #52017

Closed
tannergooding opened this issue Apr 28, 2021 · 11 comments · Fixed by #63329
Closed

Extend Vector64<T>, Vector128<T>, and Vector256<T> to support nint and nuint #52017

tannergooding opened this issue Apr 28, 2021 · 11 comments · Fixed by #63329
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.Intrinsics
Milestone

Comments

@tannergooding
Copy link
Member

Proposal

Extend Vector64<T>, Vector128<T>, and Vector256<T> to support nint and nuint as valid primitive types. This will extend a number of existing generic functions which take a Vector<T> to also support taking the new types rather than throwing a PlatformNotSupportedException.

Additionally, the following non-generic APIs should be added for parity with the existing surface area:

namespace System.Runtime.Intrinsics
{
    public static partial class Vector64
    {
        public static Vector64<nint> AsNInt<T>(Vector64<T> value);
        public static Vector64<nuint> AsNUInt<T>(Vector64<T> value);

        public static Vector64<nint> Create(nint value);
        public static Vector64<nuint> Create(nuint value);

        public static Vector64<nint> CreateScalar(nint value);
        public static Vector64<nuint> CreateScalar(nuint value);

        public static Vector64<nint> CreateScalarUnsafe(nint value);
        public static Vector64<nuint> CreateScalarUnsafe(nuint value);
    }

    public static partial class Vector128
    {
        public static Vector128<nint> AsNInt<T>(Vector128<T> value);
        public static Vector128<nuint> AsNUInt<T>(Vector128<T> value);

        public static Vector128<nint> Create(nint value);
        public static Vector128<nuint> Create(nuint value);

        public static Vector128<nint> Create(Vector64<nint> lower, Vector64<nint> upper);
        public static Vector128<nuint> Create(Vector64<nuint> lower, Vector64<nuint> upper);

        public static Vector128<nint> CreateScalar(nint value);
        public static Vector128<nuint> CreateScalar(nuint value);

        public static Vector128<nint> CreateScalarUnsafe(nint value);
        public static Vector128<nuint> CreateScalarUnsafe(nuint value);
    }

    public static partial class Vector256
    {
        public static Vector256<nint> AsNInt<T>(Vector256<T> value);
        public static Vector256<nuint> AsNUInt<T>(Vector256<T> value);

        public static Vector256<nint> Create(nint value);
        public static Vector256<nuint> Create(nuint value);

        public static Vector256<nint> Create(Vector128<nint> lower, Vector128<nint> upper);
        public static Vector256<nuint> Create(Vector128<nuint> lower, Vector128<nuint> upper);

        public static Vector256<nint> CreateScalar(nint value);
        public static Vector256<nuint> CreateScalar(nuint value);

        public static Vector256<nint> CreateScalarUnsafe(nint value);
        public static Vector256<nuint> CreateScalarUnsafe(nuint value);
    }
}
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Runtime.Intrinsics untriaged New issue has not been triaged by the area owner labels Apr 28, 2021
@ghost
Copy link

ghost commented Apr 28, 2021

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

Issue Details

Proposal

Extend Vector64<T>, Vector128<T>, and Vector256<T> to support nint and nuint as valid primitive types. This will extend a number of existing generic functions which take a Vector<T> to also support taking the new types rather than throwing a PlatformNotSupportedException.

Additionally, the following non-generic APIs should be added for parity with the existing surface area:

namespace System.Runtime.Intrinsics
{
    public static partial class Vector64
    {
        public static Vector64<nint> AsNInt<T>(Vector64<T> value);
        public static Vector64<nuint> AsNUInt<T>(Vector64<T> value);

        public static Vector64<nint> Create(nint value);
        public static Vector64<nuint> Create(nuint value);

        public static Vector64<nint> CreateScalar(nint value);
        public static Vector64<nuint> CreateScalar(nuint value);

        public static Vector64<nint> CreateScalarUnsafe(nint value);
        public static Vector64<nuint> CreateScalarUnsafe(nuint value);
    }

    public static partial class Vector128
    {
        public static Vector128<nint> AsNInt<T>(Vector128<T> value);
        public static Vector128<nuint> AsNUInt<T>(Vector128<T> value);

        public static Vector128<nint> Create(nint value);
        public static Vector128<nuint> Create(nuint value);

        public static Vector128<nint> Create(Vector64<nint> lower, Vector64<nint> upper);
        public static Vector128<nuint> Create(Vector64<nuint> lower, Vector64<nuint> upper);

        public static Vector128<nint> CreateScalar(nint value);
        public static Vector128<nuint> CreateScalar(nuint value);

        public static Vector128<nint> CreateScalarUnsafe(nint value);
        public static Vector128<nuint> CreateScalarUnsafe(nuint value);
    }

    public static partial class Vector256
    {
        public static Vector256<nint> AsNInt<T>(Vector256<T> value);
        public static Vector256<nuint> AsNUInt<T>(Vector256<T> value);

        public static Vector256<nint> Create(nint value);
        public static Vector256<nuint> Create(nuint value);

        public static Vector256<nint> Create(Vector128<nint> lower, Vector128<nint> upper);
        public static Vector256<nuint> Create(Vector128<nuint> lower, Vector128<nuint> upper);

        public static Vector256<nint> CreateScalar(nint value);
        public static Vector256<nuint> CreateScalar(nuint value);

        public static Vector256<nint> CreateScalarUnsafe(nint value);
        public static Vector256<nuint> CreateScalarUnsafe(nuint value);
    }
}
Author: tannergooding
Assignees: -
Labels:

area-System.Runtime.Intrinsics, untriaged

Milestone: -

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Apr 28, 2021
@tannergooding
Copy link
Member Author

#52021 represents the x86 ISA changes that would be appropriate

@tannergooding
Copy link
Member Author

#52027 represents the Arm ISA changes that would be appropriate

@gfoidl
Copy link
Member

gfoidl commented Apr 29, 2021

Hm, maybe I miss something now, but in simple words nint will either be int or long* depending on the bitness of the executing platform. So this would result in either Vector128<int> or Vector128<long>.
Can you give me a use case please? I can't puzzle together anything useful for this right now.

* please remember: that's in simple words 😉

@tannergooding
Copy link
Member Author

We use nint/nuint in some of our algorithms, such as https://source.dot.net/#System.Private.CoreLib/Utf16Utility.Validation.cs,359.

64-bit (long/ulong) intrinsics tend to not be supported or less efficient on 32-bit machines. While 32-bit (int/uint) intrinsics tend to not fully utilize the CPU on 64-bit machines. Using nint allows the "natural" data type to be used and can make the overall algorithm more efficient.

@gfoidl
Copy link
Member

gfoidl commented Apr 29, 2021

Got it. Thanks (nice example linked).

@bartonjs
Copy link
Member

bartonjs commented May 27, 2021

Video

Looks good as proposed. A very forward looking question of "what about when nint is bigger than 64 bits?" was asked, and deferred to such a hypothetical date.

namespace System.Runtime.Intrinsics
{
    public static partial class Vector64
    {
        public static Vector64<nint> AsNInt<T>(Vector64<T> value);
        public static Vector64<nuint> AsNUInt<T>(Vector64<T> value);

        public static Vector64<nint> Create(nint value);
        public static Vector64<nuint> Create(nuint value);

        public static Vector64<nint> CreateScalar(nint value);
        public static Vector64<nuint> CreateScalar(nuint value);

        public static Vector64<nint> CreateScalarUnsafe(nint value);
        public static Vector64<nuint> CreateScalarUnsafe(nuint value);
    }

    public static partial class Vector128
    {
        public static Vector128<nint> AsNInt<T>(Vector128<T> value);
        public static Vector128<nuint> AsNUInt<T>(Vector128<T> value);

        public static Vector128<nint> Create(nint value);
        public static Vector128<nuint> Create(nuint value);

        public static Vector128<nint> Create(Vector64<nint> lower, Vector64<nint> upper);
        public static Vector128<nuint> Create(Vector64<nuint> lower, Vector64<nuint> upper);

        public static Vector128<nint> CreateScalar(nint value);
        public static Vector128<nuint> CreateScalar(nuint value);

        public static Vector128<nint> CreateScalarUnsafe(nint value);
        public static Vector128<nuint> CreateScalarUnsafe(nuint value);
    }

    public static partial class Vector256
    {
        public static Vector256<nint> AsNInt<T>(Vector256<T> value);
        public static Vector256<nuint> AsNUInt<T>(Vector256<T> value);

        public static Vector256<nint> Create(nint value);
        public static Vector256<nuint> Create(nuint value);

        public static Vector256<nint> Create(Vector128<nint> lower, Vector128<nint> upper);
        public static Vector256<nuint> Create(Vector128<nuint> lower, Vector128<nuint> upper);

        public static Vector256<nint> CreateScalar(nint value);
        public static Vector256<nuint> CreateScalar(nuint value);

        public static Vector256<nint> CreateScalarUnsafe(nint value);
        public static Vector256<nuint> CreateScalarUnsafe(nuint value);
    }
}

@bartonjs bartonjs 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 May 27, 2021
@tannergooding tannergooding modified the milestones: 6.0.0, 7.0.0 Jul 12, 2021
@deeprobin
Copy link
Contributor

deeprobin commented Dec 21, 2021

You're welcome to assign me 😄

Edit: I was bored and couldn't wait for the issue to be assigned to me.... Therefore I have created a PR

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 26, 2021
@deeprobin
Copy link
Contributor

What puzzles me a bit about the proposal is the naming of the methods.
It is usual in the class that the methods are named after the underlying struct. But in this case it was named after the primitive.

-        public static Vector64<nint> AsNInt<T>(Vector64<T> value);
-        public static Vector64<nuint> AsNUInt<T>(Vector64<T> value);
+        public static Vector64<nint> AsIntPtr<T>(Vector64<T> value);
+        public static Vector64<nuint> AsUIntPtr<T>(Vector64<T> value);

Have you considered this?

@tannergooding
Copy link
Member Author

It is usual in the class that the methods are named after the underlying struct

NInt means it takes nint and IntPtr means it takes IntPtr and due to the current language differences, that semantic can be meaningful. It may change in the future, particularly due to generic math, but as far as API names go; that's the meaning/intent and how other already shipped APIs differentiate.

@tannergooding
Copy link
Member Author

Edit: I was bored and couldn't wait for the issue to be assigned to me.... Therefore I have created a PR

(just posting our Discord conversation for completeness).

I was on vacation and not checking work related GitHub so this got missed. I've actually already done the work here and its in a local branch waiting for other PRs, such as #61649 to get merged before it gets put up.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 4, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 4, 2022
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.Runtime.Intrinsics
Projects
None yet
4 participants