Memory Resource / View

[mzient]

Design: #129 (proposal from a comment)

This work builds on #105

The resource is defined in terms of memory kind. resource_view (a glorifieid pointer with elaborate conversion logic) is defined in terms of the properties of the memory allocated. This allows, for example, to define a function that expects a resource_view<memory_access::host> to take memory_resource<host> and memory_resource<managed>, even though these types are not related.

Summary (from #105): cuda::memory_kind (namespace) Groups kinds of memory allocated Possible values: device, managed, pinned, host cuda::memory_resource<memory_kind, context> Synchronous (de)allocation of storage of the specified memory_kind cuda::stream_ordered_memory_resource<memory_kind> Asynchronous, stream-ordered (de)allocation of storage of the specified memory_kind The semantics of stream ordered memory allocation are as defined here stream_ordered_memory_resource inherits from memory_resource and provides a default implementation of allocate/deallocate by allocating on the default stream and synchronizing. cuda::stream_view A non-owning wrapper for cudaStream_t cuda::cuda_error Exception thrown on CUDA runtime API errors throw_on_cuda_error utility for checking result of CUDA runtime API calls

New (wrt #105): cuda::basic_resource_view<resource_pointer, properties...> a pointer-like object that can be used in place of memory_resource and pameterized in terms of memory properties instead of memory kind. cuda::memory_resource_base, cuda::stream_ordered_memory_resource_base - base classes with common interface for all memory resources regardless of their memory kind. The bases are private so that the user can't blindly declare a function parameter as cuda::memory_resource_base and unsafely pass a resource of any kind there. Instead, resource_view should be used. resource_view, stream_ordererd_resource_view - template aliases for basic_resource_view, with pointers the base resource types substituted for the resource_pointer. These types are befriended with (stream_ordered_)memory_resource and therefore the access to the private base class is allowed. There's no way to obtain the base pointer except for calling operator-> which is intended for exposing the resource interface.

TODO: Add tests for resource_view

[harrism]

Must you force push? As you can see, doing so disassociates the comment history from the code.

[mzient]

Must you force push? As you can see, doing so disassociates the comment history from the code.

Sorry... I did it only because I rebased on main prior to merging the workaround from NVIDIA/libcudacxx#183

[wmaxey]

Must you force push? As you can see, doing so disassociates the comment history from the code.

Sorry... I did it only because I rebased on main prior to merging the workaround from NVIDIA/libcudacxx#183

Yeah, this is an unfortunate drawback to Github's PR workflow. We should probably setup some kind of etiquette or something for how we should handle this in the future.

[harrism]

Generally rebasing rather than merging is problematic with github. If you are worried about keeping the history clean, the repo should use squash merge commits when merging PRs.

[harrism]

@mzient did you see my comment here: https://github.com/NVIDIA/libcudacxx/pull/158#discussion_r669279406

I ask because the code I mentioned still segfaults with your latest commits.

// segfault
if (cuda::resource_view<cuda::memory_access::device>{nullptr} ==
      cuda::resource_view<cuda::memory_access::device>{nullptr})
    printf("equal\n");

The fix I suggested in that comment stops the segfault. But do we want two nullptr views to be considered equal, or unequal?

[harrism]

I'm struggling with implementing do_is_equal in some derived classes. In many MR / adaptor implementations, there is a stored view of an upstream resource. So I may need to compare this upstream resource view to the passed-in resource. This is effectively like this:

bool do_is_equal()cuda::memory_resource<memory_kind> const& other)
{
  return upstream_ ==     cuda::resource_view<memory_kind>{&other};
}

Where upstream_ is cuda::stream_ordered_resource_view<cuda::memory_access::device>. But this won't compile, I get errors like this:

No matching constructor for initialization of 'cuda::resource_view<cuda::memory_access::device>' (aka 'basic_resource_view<detail::memory_resource_base *, cuda::memory_access::device>')clang(ovl_no_viable_function_in_init)
memory_resource(842, 7): Candidate constructor (the implicit copy constructor) not viable: no known conversion from 'const cuda::memory_resource<cuda::memory_kind::device> *' to 'const cuda::basic_resource_view<cuda::detail::memory_resource_base *, cuda::memory_access::device>' for 1st argument
memory_resource(842, 7): Candidate constructor (the implicit move constructor) not viable: no known conversion from 'const cuda::memory_resource<cuda::memory_kind::device> *' to 'cuda::basic_resource_view<cuda::detail::memory_resource_base *, cuda::memory_access::device>' for 1st argument
memory_resource(854, 3): Candidate constructor not viable: no known conversion from 'const cuda::memory_resource<cuda::memory_kind::device> *' to 'std::nullptr_t' (aka 'nullptr_t') for 1st argument
memory_resource(870, 3): Candidate template ignored: requirement 'cuda::std::__3::conjunction<cuda::has_property<const cuda::memory_resource<cuda::memory_kind::device, cuda::any_context>, cuda::memory_access::device>>::value' was not satisfied [with _Resource = const cuda::memory_resource<cuda::memory_kind::device>]
memory_resource(889, 3): Candidate template ignored: could not match 'basic_resource_view<type-parameter-0-0, type-parameter-0-1...>' against 'const cuda::memory_resource<cuda::memory_kind::device> *'
memory_resource(852, 3): Candidate constructor not viable: requires 0 arguments, but 1 was provided

This is the relevant one, I think:

Candidate template ignored: requirement 'cuda::std::__3::conjunction<cuda::has_property<const cuda::memory_resource<cuda::memory_kind::device, cuda::any_context>, cuda::memory_access::device>>::value' was not satisfied [with _Resource = const cuda::memory_resource<cuda::memory_kind::device>]

I can't figure out how to create a view from other...

[mzient]

I'm struggling with implementing do_is_equal in some derived classes. In many MR / adaptor implementations, there is a

It boiled down to property checks not working for const-qualified memory resources. Nomrally it would be OK - after all, who'd need a const-qualified memory resource, where this removes almost all functionality? But now that we're interested in just running some checks (comparison, possibly device queries in the future) it starts to make sense. I adjusted the SFINAE checks.

[harrism]

I still can't get some derived MRs to build due to do_is_equal. The problem is that in order to create a derived memory_resource type you have to implement

bool do_is_equal(cuda::memory_resource<memory_kind> const& other) const noexcept override

But for my example derived MR type, equality comparison requires comparing the upstream resource, which is held as a cuda::stream_ordered_resource_view<cuda::memory_access::device>. But this fails to compile because you can't convert other to a stream_ordered_resource_view:

[[nodiscard]] bool do_is_equal(
    cuda::memory_resource<memory_kind> const& other) const noexcept override
  {
    if (this == &other) { return true; }
    auto const* cast = dynamic_cast<limiting_resource_adaptor const*>(&other);
    if (cast != nullptr) { return upstream_ == cast->get_upstream(); }
    return upstream_ == &other;
  }

I get an error on the last line of that function:

_deps/libcudacxx-src/include/cuda/memory_resource:840:76: error: static assertion failed: The resource views are not compatible
  840 |       static_assert(cuda::is_view_convertible<__view1_t, __view2_t>::value || cuda::is_view_convertible<__view2_t, __view1_t>::value,
      |                                                                      ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

upstream_ is defined cuda::stream_ordered_resource_view<cuda::memory_access::device> upstream_;, so I guess a cuda::memory_resource is not view convertible to `cuda::stream_ordered_resource_view.

If I could, I would change do_is_equal to take a stream_ordered_memory_resource, but I can't do that.

[harrism]

After your latest changes this still doesn't compile:

cuda::stream_ordered_resource_view<cuda::memory_access::device> view{nullptr};
cuda::stream_ordered_memory_resource<cuda::memory_kind::device>* mr{nullptr};
if (view == mr) { std::cout << "equal\n"; }

But this does:

cuda::stream_ordered_resource_view<cuda::memory_access::device> view{nullptr};
cuda::stream_ordered_memory_resource<cuda::memory_kind::device>* mr{nullptr};
if (view == cuda::view_resource(mr)) { std::cout << "equal\n"; }

[harrism]

The latter problem seems to be solved by adding overloads of operator== for stream_ordered_memory_resource like the ones you added for memory_resource. But adding these does not solve the problem with implementing do_is_equal.

template <typename _ResourcePointer, typename... _Properties, typename _Kind>
bool operator==(const basic_resource_view<_ResourcePointer, _Properties...> &__view, const stream_ordered_memory_resource<_Kind> *__mr) {
  return __view == view_resource(__mr);
}

template <typename _ResourcePointer, typename... _Properties, typename _Kind>
bool operator!=(const basic_resource_view<_ResourcePointer, _Properties...> &__view, const stream_ordered_memory_resource<_Kind> *__mr) {
  return __view != view_resource(__mr);
}

template <typename _ResourcePointer, typename... _Properties, typename _Kind>
bool operator==(const stream_ordered_memory_resource<_Kind> *__mr, const basic_resource_view<_ResourcePointer, _Properties...> &__view) {
  return view_resource(__mr) == __view;
}

template <typename _ResourcePointer, typename... _Properties, typename _Kind>
bool operator!=(const stream_ordered_memory_resource<_Kind> *__mr, const basic_resource_view<_ResourcePointer, _Properties...> &__view) {
  return view_resource(__mr) != __view;
}

[harrism]

@mzient would you mind clicking "resolved" on the comment threads above that have been resolved?

[harrism]

@mzient you also should probably resolve the branch conflict mentioned below.

[harrism]

Is there a reason that memory_resource does not have operator==? When I have references to MRs, I have to create a view of at least one of them in order to compare them.

[mzient]

@mzient would you mind clicking "resolved" on the comment threads above that have been resolved?

I could, but I've always thought that it's the reporter of the issue who considers it resolved and clicks it. Actually, we consider it a violation of our project etiquette in DALI for anyone else than OP to click it...

@mzient you also should probably resolve the branch conflict mentioned below. Will do

Is there a reason that memory_resource does not have operator==? When I have references to MRs, I have to create a view of at least one of them in order to compare them.

Not really. We could add it with relative ease, I think. It would come with the perk that it would be impossible to compare resources of different kind.

[harrism]

Must you force push? As you can see, doing so disassociates the comment history from the code.

Sorry... I did it only because I rebased on main prior to merging the workaround from NVIDIA/libcudacxx#183

Yeah, this is an unfortunate drawback to Github's PR workflow. We should probably setup some kind of etiquette or something for how we should handle this in the future.

Really need to start merging rather than rebasing, and then use squash merging into the main/release branch. Then you get the clean git history benefit of rebasing without screwing up the PR history and disassociating reviews from the code. FWIW, this is what RAPIDS does and it works great (we didn't used to use squash merges, so our git history was messy. But now we do and all is good).

[raphaelreinauer]

I wanted to ask what the current status is. I want to use the memory resource for my project.

[jrhemstad]

Superseded by https://github.com/NVIDIA/libcudacxx/pull/309