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 : Arm64 Load & Store #24771

Closed
sdmaclea opened this issue Jan 23, 2018 · 20 comments · Fixed by #33535
Closed

API Proposal : Arm64 Load & Store #24771

sdmaclea opened this issue Jan 23, 2018 · 20 comments · Fixed by #33535
Assignees
Labels
api-approved API was approved in API review, it can be implemented arch-arm64 area-System.Runtime.Intrinsics
Milestone

Comments

@sdmaclea
Copy link
Contributor

sdmaclea commented Jan 23, 2018

Edit: Removed StaticCast as that is handled by Vector128.As*. Kept the Load/Store APIs as generic with note that the actual implementation is exploded and supports all 10 primitive types

namespace System.Runtime.Intrinsics.Arm
{
    public abstract class AdvSimd 
    {
        public abstract class Arm64
        {
            /// <summary>
            /// Vector load
            ///
            /// Corresponds to vector form of ARM64 LDR
            /// </summary>
            public static unsafe Vector64<T>  LoadVector64<T>(void* address) where T : struct { throw null; }
            public static unsafe Vector128<T> LoadVector128<T>(void* address) where T : struct { throw null; }
    
            /// <summary>
            /// Vector store
            ///
            /// Corresponds to vector form of ARM64 STR
            /// </summary>
            public static unsafe void Store<T>(void* address, Vector64<T>  source) where T : struct { throw null; }
            public static unsafe void Store<T>(void* address, Vector128<T> source) where T : struct { throw null; }
        }
    }
}
@sdmaclea
Copy link
Contributor Author

These are similar to similarly named methods in X86.

The unsafe Load* and Store are basically identical to X86.

X86 has StaticCast which I suspect is what I have called ReinterpretCast. Static cast seems to imply conversion. Since I intend no conversion, this seemed to be the correct name.

@eerhardt @CarolEidt @RussKeldorph PTAL
@tannergooding @4creators @dotnet/arm64-contrib @dotnet/jit-contrib @fiigii FYI

@fiigii
Copy link
Contributor

fiigii commented Jan 23, 2018

public static unsafe Vector64 LoadVector64(T* address) where T : struct { throw null; }

As I know, C# does not have pointers to generic types.

@sdmaclea
Copy link
Contributor Author

C# does not have pointers to generic types.

Interesting... I think it is reasonable to review as is with the understanding that these may need to expand.

@fiigii
Copy link
Contributor

fiigii commented Jan 23, 2018

X86 has StaticCast which I suspect is what I have called ReinterpretCast. Static cast seems to imply conversion. Since I intend no conversion, this seemed to be the correct name.

I am not sure if we should match the C++ name (actually, I do not think static_cast is a good name in C++ when it is used on value-types).

When I designed the API, I thought StaticCast<T, U> means that cast the type to make the static type system happy and no any dynamic conversion.

I am happy to hear more comments from C# experts. cc @jkotas @AndyAyersMS @benaadams

@4creators
Copy link
Contributor

When I designed the API, I thought StaticCast<T, U> means that cast the type to make the static type system happy

IMO bcs not only of the reasons above @fiigii naming is more appealing to C# coders. My first impression with the name is that it does provide casting operations based on static methods and essentially it does that. There is StaticDelegate proposal which implies static, thin, fast delegates and in it's meaning it is close to StaticCast which is also fast (zero op), thin cast. But I could be biased ...

@jkotas
Copy link
Member

jkotas commented Jan 23, 2018

StaticCast

The equivalent APIs for Span is going to be called just Cast: Span<TTo> Cast<TFrom, TTo>(Span<TFrom> source).

dotnet/coreclr#15941

@sdmaclea
Copy link
Contributor Author

Sounds like X86 StaticCast<> is a zero op. We definitely need to match. I changed above to StaticCast.

If we should match Span<>.Cast<>, I am happy with that too.

@sdmaclea
Copy link
Contributor Author

C# does not have pointers to generic types

I change the T* to void*. This could also be Vector64<void>* or something like that.

@sdmaclea
Copy link
Contributor Author

@eerhardt @CarolEidt @RussKeldorph API-Ready-For-Review?

@eerhardt
Copy link
Member

The x86 LoadVector methods are expanded out:

ex.

        public static unsafe Vector128<sbyte> LoadVector128(sbyte* address) { throw null; }
        public static unsafe Vector128<byte> LoadVector128(byte* address) { throw null; }
        public static unsafe Vector128<short> LoadVector128(short* address) { throw null; }
        public static unsafe Vector128<ushort> LoadVector128(ushort* address) { throw null; }
        public static unsafe Vector128<int> LoadVector128(int* address) { throw null; }
        public static unsafe Vector128<uint> LoadVector128(uint* address) { throw null; }
        public static unsafe Vector128<long> LoadVector128(long* address) { throw null; }
        public static unsafe Vector128<ulong> LoadVector128(ulong* address) { throw null; }
        public static unsafe Vector128<double> LoadVector128(double* address) { throw null; }

Same for Sse2.Store.

Why are we using generics and void* here?

@sdmaclea
Copy link
Contributor Author

Why are we using generics and void* here?

Only to reduce API surface. I am happy to change.

@eerhardt
Copy link
Member

I would prefer the two sets of APIs be consistent, unless there is a reason/value for them to be different.

@tannergooding
Copy link
Member

I believe in one of the discussions with @CarolEidt, it was determined that we could/should use generics if all 10 numeric types for a method are supported in a single ISA.

x86 doesn't meet this requirement in a number of places, since it generally exposes a subset in one ISA and the rest in the next iteration (e.g. float in Sse, and the other 9 in Sse2, or float and double in Avx, but the other 8 in Avx2).

x86 is using generics in the few places where it does meet the first requirement.

@eerhardt
Copy link
Member

OK - that makes sense to me why they are different.

@sdmaclea
Copy link
Contributor Author

sdmaclea commented Feb 5, 2018

@eerhardt Any reason why this is marked future? I believe this is within range of 2.1. Implementation should take more than a few days.

@sdmaclea
Copy link
Contributor Author

@eerhardt @CarolEidt @RussKeldorph

These are not currently implemented. I think they their absence will make the Arm64 SIMD intrinsics difficult to use. I think we could safely implement them in CoreCLR and expose the API in CoreFX in time for Zero Bug Bounce.

Is that still an option? Or will the PR's be deferred till after ZBB.

@eerhardt
Copy link
Member

If we can't complete simple scenarios with the ARM64 intrinsics APIs, then it doesn't make sense to leave them in. So I'd suggest at least getting a simple end-to-end scenario working, or removing the APIs all together.

Is that still an option?

I would take the PRs to complete a simple end-to-end scenario if they were completed before ZBB.

@terrajobst
Copy link
Member

Should the naming reflect alignment requirements?
- LDR doesn't require alignment
- LDX does require alignment

  • We probably don't need the static cast as this is just type-system change, so maybe we should just move this to a generic method the vector types.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@tannergooding tannergooding added api-ready-for-review and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Feb 10, 2020
@tannergooding tannergooding changed the title API Proposal : Arm64 Load, store & cast API Proposal : Arm64 Load & Store Feb 10, 2020
@tannergooding
Copy link
Member

The LoadVector* APIs have already been implemented, we just need to review/approve the surface area.

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review labels Feb 18, 2020
@terrajobst
Copy link
Member

Looks good as proposed, except the namespaces/types should match the existing types:

namespace System.Runtime.Intrinsics.Arm
{
    public partial class AdvSimd 
    {
        public partial class Arm64 : ArmBase
        {
            /// <summary>
            /// Vector load
            ///
            /// Corresponds to vector form of ARM64 LDR
            /// </summary>
            public static unsafe Vector64<T>  LoadVector64<T>(void* address) where T : struct;
            public static unsafe Vector128<T> LoadVector128<T>(void* address) where T : struct;
        
            /// <summary>
            /// Vector store
            ///
            /// Corresponds to vector form of ARM64 STR
            /// </summary>
            public static unsafe void Store<T>(void* address, Vector64<T>  source) where T : struct;
            public static unsafe void Store<T>(void* address, Vector128<T> source) where T : struct;
        }
    }
}

@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2020
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 arch-arm64 area-System.Runtime.Intrinsics
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants