Add managed_pointer that is compatible with STL.

[alliepiper]

The existing cuda::pointer uses a fancy reference that overloads
operator&, and some STL implementations misbehave when that operator
does not return the actual memory address of the object.

Since universal_memory_resource allocates memory that works on both host
and device, we need to be able to use these types with stl containers,
such as std::vector.

This patch adds a managed_pointer implementation that behaves like
cuda::pointer, but returns a regular c++ reference, allowing
the thrust universal allocator to work with STL containers.

[alliepiper]

Internal shelve: 28110928.

[brycelelbach]

managed_pointer is definitely the wrong name for this; it suggests that it is a smart pointer and will clean up the object itself.

Please call it universal_pointer.

[jaredhoberock]

Please call it universal_pointer.

I think the right name is managed_ptr. It's allocated by cudaMallocManaged. "Universal" doesn't mean anything to CUDA programmers. The _ptr suffix should also be used for consistency.

Others have implemented this functionality in the past, and it's been consistently referred to as "managed" pointer.

[griwes]

..._pointer is the right naming for this, as an analogy to the pointer class template in the same namespace (device_ptr is a different abstraction layer).

[brycelelbach]

Is this only in the cuda namespace? Sent from Nine<http://www.9folders.com/>

…

________________________________ From: Jared Hoberock <notifications@github.com> Sent: Friday, March 13, 2020 17:07 To: thrust/thrust Cc: Bryce Lelbach; Review requested Subject: Re: [thrust/thrust] Add managed_pointer that is compatible with STL. (#1068) Please call it universal_pointer. I think the right name is managed_ptr. It's allocated by cudaMallocManaged. "Universal" doesn't mean anything to CUDA programmers. The _ptr suffix should also be used for consistency. Others have implemented this functionality in the past, and it's been consistently referred to as "managed" pointer. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub<#1068 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AADBG4WGAWZRKQVKXJEUWH3RHLDCPANCNFSM4LF6F3GA>.

----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------

[alliepiper]

It's in a cuda specific detail namespace as an implementation detail of the cuda managed_memory_resource.

[jaredhoberock]

OK, if it's in a detail namespace, it doesn't really matter what it's called :-)

[brycelelbach]

We should have a top-level way of spelling the pointer type returned by thrust::universal_allocator; it should be called thrust::universal_pointer or thrust::universal_ptr (@griwes will have an opinion about which of those two names is more correct, I suspect the later).

[brycelelbach]

Should we have a managed_ptr in each backend?

[alliepiper]

Ya'll seem to have much stronger opinions on this than I do, so let's see if we can find a color for this bikeshed that we'll all be happy with 😄If I'm following correctly:

• Jared: It doesn't really matter since it's a detail, but prefer managed_... to universal_...
• Michal: Just make sure it's ..._pointer and not ...ptr to prevent confusion.
• Bryce: managed_... makes it sound like it does resource management and might be confusing.
• Allie: 🤷‍♀

How about we go with detail::managed_memory_pointer<T>, I think that would be agreeable with everyone?

[alliepiper]

We should have a top-level way of spelling the pointer type returned by thrust::universal_allocator; it should be called thrust::universal_pointer or thrust::universal_ptr (@griwes will have an opinion about which of those two names is more correct, I suspect the later).

As it stands, thrust::universal_allocator<T> doesn't exist. It currently has to be spelled out explicitly, as thrust::mr::stateless_resource_allocator<T, thrust::universal_memory_resource>. Perhaps we should add universal_allocator<T> typedefs to simplify things for users?

And if we had that, I think thrust::universal_allocator<T>::pointer would be sufficient for spelling the pointer type. I don't think we really need to add a specific pointer typedef for this to thrust::.

[jaredhoberock]

If the purpose of this allocator is to be useful in any Thrust backend, I'd prefer it simply be named thrust::allocator (if it must be named at all).

That naming makes it obvious what it does when CUDA is not involved.

[alliepiper]

Only concern I have there is that we're already using the term "universal" outside of cuda, eg. thrust::universal_memory_resource in thrust/memory/detail/device_system_resource.h. In that context, universal_allocator is more consistent IMO.

[brycelelbach]

Yes, we need a top level alias for the allocator and for the pointer type.

I'd suggest a quick meeting to discuss this.

[alliepiper]

@brycelelbach this is blocked on your change request.

New Shelve: 28165473