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] Support for constructing and copying Vector<T> from pointers #16026

Closed
mellinoe opened this issue Jan 6, 2016 · 24 comments
Closed
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Numerics enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@mellinoe
Copy link
Contributor

mellinoe commented Jan 6, 2016

Summary

Currently, the System.Numerics.Vectors library provides no way to construct Vector<T> objects out of native pointers, or from anything other than an array or a single constant element. Although part of the draw of this library is access to high-performance vector math without resorting to native or unsafe code, there’s a lot of reasons you might have your data stored somewhere outside of a managed array. It’s not reasonable to copy all of your data into an array just to do vector operations on it, and then copy it back to its original location. Additionally, you might want to construct a one-off vector from the stack (perhaps using stackalloc), without having to make an entire array for the purpose of a single vector. Even if you have a safe IntPtr to your storage location, you cannot currently construct a Vector<T> from it.

We should expose better support for, at the very least, constructing Vector<T>’s from arbitrary pointers, and storing them back into arbitrary locations.

Proposed API

public struct Vector<T>
{
    public Vector(void* dataPointer);
    public Vector(void* dataPointer, int offset);
    public Vector(IntPtr dataPointer);
    public Vector(IntPtr dataPointer, int offset);

    public void CopyTo(void* destination);
    public void CopyTo(void* destination, offset);
    public void CopyTo(IntPtr destination);
    public void CopyTo(IntPtr destination, offset);
}

Because all pointer types can be implicitly converted to a void* pointer, you can use the first overloads above to load a Vector<float> by passing a float* for example. It also allows you to load a Vector<float> with any other pointer type, but in my opinion this is not a problem. Even with the alternate approach below (see “Alternative Approaches”), you can simply cast a pointer type to another and construct any kind of Vector<T> you would like. This is desirable in some situations if you want to perform different calculations or reinterpret your data in some way.

Constructor

This behaves exactly the same way as the constructor taking an array, and allows an optional offset to be given in order to skip elements at the beginning of your storage.

CopyTo

This behaves exactly the same way as CopyTo(T[]), also with an optional offset to control the storage location.

JIT Recognition

These new overloads must be recognized by the JIT. Functionally, they are pretty much identical to the existing overloads which operate on arrays.

Alternative Approach

Instead of a “universal” pointer constructor, we could expose some “factory” methods which take a specifically-typed pointer and return a specifically typed Vector<T>.

public static Vector<byte> Load(byte* dataPointer, int offset);
public static Vector<int> Load(int* dataPointer, int offset);
public static Vector<float> Load(float* dataPointer, int offset);
...

I don’t personally favor this approach as using static methods for construction doesn’t necessarily mesh with the existing usage patterns, and it also just adds a lot more methods than constructors (two per primitive type, so 20 in total). Also, It would still make sense to add the IntPtr constructor even if we went this route.

Related to https://github.com/dotnet/corefx/issues/5106, https://github.com/dotnet/corefx/issues/3741

I have part of the additions (void* overloads) implemented in this branch with tests added.

Clarifications

Q: Since Vector<T> is a struct, why do we need special constructors for pointers pointers? Other BCL structures don't need special constructors for this, you can just cast, de-reference, etc.

Because Vector<T> is a generic struct, you can't directly take it's address, nor have a pointer to it.

error CS0208: Cannot take the address of, get the size of, or declare a pointer to a managed type ('Vector<float>')

Consider Vector4, which is sort of similar to Vector<float>. If you want to get a Vector4 from a float* or vice-versa, you can just do the following, and you don't need a specialized constructor for it:

float* data = stackalloc float[4];
Vector4 vec = *(Vector4*)data;

Vector4* vectorData = stackalloc Vector4[100];
Vector4 first = vectorData[0];
float* xPtr = &first.X;
// etc.

Because Vector<T> is generic, the above mechanisms aren't applicable and you're much more limited in how you can constructor and store them with pointers.

@mellinoe
Copy link
Contributor Author

mellinoe commented Jan 6, 2016

@terrajobst , @CarolEidt

@stephentoub
Copy link
Member

I wonder whether we actually want to expose the void* overloads. This may just be a nit, but in doing so we'd make it easy to accidentally take something inherently unsafe and wrap it in something that isn't unsafe such that it could easily escape the unsafe context (e.g. creating a Vector<T> from some stackalloc'd state and then returning that Vector<T> from the method). With the IntPtr overloads, you'd at least need to explicitly cast from a void* to an IntPtr, making it a bit more obvious that something dangerous is potentially being done. Just food for thought.

@mellinoe
Copy link
Contributor Author

mellinoe commented Jan 6, 2016

I think I understand where you're coming from, but I don't think it's an issue, actually. The constructor copies the data out of the pointer's location, it doesn't actually wrap it. By the time you return the Vector<T> from your unsafe method, the stackalloc'd memory wouldn't be read again, and the struct returned from the method just has a copy of the data. Let me give an example:

public unsafe Vector<float> GetRandomFloats()
{
    Random r = new Random();
    float* data = stackalloc float[Vector<float>.Count];
    for (int i = 0; i < Vector<float.Count; i++)
        data[i] = (float)r.NextDouble();
    return new Vector<float>(data);
}

Vector<T> is a struct so you will just get back a copy of that data when the method returns. Does that make sense?

@stephentoub
Copy link
Member

It copies the data??

  1. Then why are these ctors useful at all? That implies the struct needs to heap-allocate storage, do the copy, etc., and that could all be done by the caller.
  2. How does the ctor know how much data to copy? You're giving it a pointer without a length.

EDIT: Oh, wait, I see, Vector<T> is fixed-length.

@CarolEidt
Copy link
Contributor

It doesn't copy the data into an object - it copies the data into a struct. Which, with the intrinsic, will usually mean copying the data from memory (wherever it's been allocated) into a SIMD register. As for how much to copy, it will always copy the length of a Vector, which the caller would presumably ensure is accessible.

@stephentoub
Copy link
Member

Yes, I was misunderstanding the design of Vector<T>... I didn't realize it was fixed in length based on the current architecture, and instead thought it was essentially a wrapper for a T[] / T* with a length.

@mellinoe
Copy link
Contributor Author

mellinoe commented Jan 6, 2016

Carol put it better than I did, I think. I will include that information in the proposal because it's important.

Another thing that might help explain why these will be useful:

Because Vector<T> is a generic struct, you can't directly take it's address, nor have a pointer to it.

error CS0208: Cannot take the address of, get the size of, or declare a pointer to a managed type ('Vector<float>')

Consider Vector4, which is sort of similar to Vector<float>. If you want to get a Vector4 from a float* or vice-versa, you can just do the following, and you don't need a specialized constructor for it:

float* data = stackalloc float[4];
Vector4 vec = *(Vector4*)data;

Vector4* vectorData = stackalloc Vector4[100];
Vector4 first = vectorData[0];
float* xPtr = &first.X;
// etc.

Because Vector<T> is generic, the above mechanisms aren't applicable and you're much more limited in how you can constructor and store them with pointers.

@terrajobst
Copy link
Member

@stephentoub I assume the comments from @CarolEidt and @mellinoe clarified it. Are you still opposed to the design?

@stephentoub
Copy link
Member

Are you still opposed to the design?

No, it's fine.

@terrajobst
Copy link
Member

Couple of questions:

  1. Do we need the IntPtr overloads at all?
  2. You say, the JIT will support those intrinsics. What does that mean for .NET Native?
  3. I assume for the time being, the intrinsic will only be supported by CoreCLR. What happens when these APIs are used on the desktop CLR? Will they work but not be intrinsics or will they fail?

@terrajobst
Copy link
Member

Also, tagging a few more folks that might have input. If possible, we should try to conclude it here, as opposed to having a meeting.

@weshaggard @KrzysztofCwalina @ericstj

@mellinoe
Copy link
Contributor Author

mellinoe commented Jan 7, 2016

Do we need the IntPtr overloads at all?

We don't need them, but it would let you stay out of unsafe code if you already have an IntPtr to your data. System.Drawing.Bitmap, for example, exposes an IntPtr to the pixel data which you could operate on without entering an unsafe context. Without those overloads, all you'd have to do is call ToPointer(), but might force you to adopt unsafe code where you previously had none.

You say, the JIT will support those intrinsics. What does that mean for .NET Native?

Right now, .NET Native doesn't support the intrinsics at all, as far as I'm aware. It's only available on RyuJIT, and LLILC has some support for it as well.

I assume for the time being, the intrinsic will only be supported by CoreCLR. What happens when these APIs are used on the desktop CLR? Will they work but not be intrinsics or will they fail?

The support is actually from RyuJIT, so it will work on the (x64) desktop CLR when the JIT is refreshed in the next framework update. Until then, they will work but not be intrinsics. I believe that the void* overload is already recognized by the JIT (correct me if I'm wrong, @CarolEidt ) as we already use it in the IL implementation in some cases.

@CarolEidt
Copy link
Contributor

I should probably know the answer to this, but would void* vs IntPtr be distinct overloads, since (I think) both would map to native int in the IL?

@CarolEidt
Copy link
Contributor

The void* overload is not yet recognized by the JIT, but these would be very easy to add.

@mellinoe
Copy link
Contributor Author

mellinoe commented Jan 7, 2016

but would void* vs IntPtr be distinct overloads, since (I think) both would map to native int in the IL?

They will be distinct, since you can have both overloads. IntPtr maps to native int in IL, but void* maps to void*.

Compiling this:

public unsafe void VoidPtr(void* ptr)
{

}

public unsafe void IntPtr(IntPtr ptr)
{

}

gives this IL (from ILSpy):

.method public hidebysig 
    instance void VoidPtr (
        void* ptr
    ) cil managed 
{
    // Method begins at RVA 0x2050
    // Code size 2 (0x2)
    .maxstack 8

    IL_0000: nop
    IL_0001: ret
} // end of method Program::VoidPtr

.method public hidebysig 
    instance void IntPtr (
        native int ptr
    ) cil managed 
{
    // Method begins at RVA 0x2050
    // Code size 2 (0x2)
    .maxstack 8

    IL_0000: nop
    IL_0001: ret
} // end of method Program::IntPtr

@mellinoe
Copy link
Contributor Author

Any more opinions regarding this? I'd like to get this in for RC2 but the deadline for that is quickly approaching. I'll have the managed-side implementation complete sometime in the next day or so (just need to add the IntPtr overloads assuming we think they are valuable).

@mellinoe
Copy link
Contributor Author

Update: My working branch now has all of the proposed overloads implemented. On the manged side, the IntPtr overloads just delegate to the void* overloads.

@krwq
Copy link
Member

krwq commented Jan 12, 2016

Does it make sense to wait for Span<T>? That would unify array/pointer experience
cc: @KrzysztofCwalina

@benaadams
Copy link
Member

Just to follow up on the api review; fine without the index/offset param for void*.

If safety checks were desirable, taking count wouldn't work so well as people would just pass in Vector<T>.Count and would ignore the differences with hardware. However, taking something like destinationSizeInBytes and using it as a bounds check might work better.

Though if you are going unsafe people like no bounds checks and in unsafe you are already at own risk; so you've generally already done them; otherwise you likely already in a lot of trouble.

However, it might make sense for the IntPtr overloads if they remain? Then throw if destinationSizeInBytes < Vector<T>.Count * sizeof(T) - though it would at most be a gesture (as it is in MemoryCopy)

e.g.

public Vector(IntPtr dataPointer, int offset, int destinationSizeInBytes);
public void CopyTo(IntPtr destination, int offset, int destinationSizeInBytes);

As per precedent with Buffer.MemoryCopy

public static unsafe void MemoryCopy(void* source, void* destination, long destinationSizeInBytes, long sourceBytesToCopy)

For our use case it would be void* rather than IntPtr so am ok if these apis are dropped.

@KrzysztofCwalina
Copy link
Member

@krwq, I don't think we should block on Span. It's unclear when we will get it and also we already have so many APIs in the framework done the "old way" that adding one more won't make a substantial difference.

@terrajobst
Copy link
Member

We've approved the API with feedback. Details in the notes.

@nietras
Copy link
Contributor

nietras commented Jan 15, 2016

We would love this for use in image processing, where we always have unmanaged memory as source/destination, so looking forward to this. And preferably with no checks, since we would use this inside a loop where we are already sure we are within bounds.

@benaadams
Copy link
Member

@nietras you may want to provide feedback on the api review dotnet/apireviews#23 as there is still some debate on the exact api surface

@terrajobst
Copy link
Member

After extension discussion we decided against these APIs. See notes for more details.

This feature will be replaced by dotnet/corefx#5474.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 1.0.0-rc2 milestone Jan 31, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Jan 3, 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 enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

9 participants