rmm icon indicating copy to clipboard operation
rmm copied to clipboard

[WIP] Interface rework

Open mzient opened this issue 4 years ago • 9 comments

This is a proposal for the interface rework that would satisfy the requirements outlined in #660

The changes are:

  • add notion of memory_kind and allocation_order
  • add a base class that mimics pmr::memory_resource, but is templated with respect to memory_kind and allocation_order
  • add a base class stream_aware_memory_resource which covers the functionality previously implemented by device_memory_resource, but with configurable memory_kind
  • make host_memory_resource an alias for memory_resource<memory_kind::host, allocation_order::host>
  • make device_memory_resource derive from stream_aware_resource<memory_kind::device>
  • add synchronous calls to stream_aware_memory_resource by inheriting from memory_resource<kind, allocation_order::host>
  • add default implementation for synchronous allocations
  • remove default stream argument
  • add default_alignment<memory_kind> constant, which resolves to alignof(max_align_t) for host and 256B for others

Behavior changes:

  • a call to stream_aware_memory_resource::allocate without a stream will cause synchronization (to run on default stream, user needs to explicitly specify the default stream)

Breaking changes:

  • do_allocate and do_deallocate in stream_aware_memory_resource have been renamed to avoid an unfixable warning when compiling with NVCC (see below)
  • do_alloc_async / do_dealloc_async now accept an alignment

NVCC warning problem:

Overriding a subset of virtual function's overloads causes a warning about hiding a virtual function. It's impossible to disable this warning or to mitigate it in the code, because the functions involved are private and therefore not accessible in the derived class - so we can't add dummy overrides or add using base::do_allocate.

mzient avatar Dec 17 '20 08:12 mzient

Can one of the admins verify this patch?

GPUtester avatar Dec 17 '20 08:12 GPUtester

Can one of the admins verify this patch?

GPUtester avatar Dec 17 '20 08:12 GPUtester

Can one of the admins verify this patch?

GPUtester avatar Dec 17 '20 08:12 GPUtester

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

github-actions[bot] avatar Mar 31 '21 10:03 github-actions[bot]

@mzient can we close this?

harrism avatar Apr 01 '21 00:04 harrism

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

github-actions[bot] avatar May 09 '21 16:05 github-actions[bot]

@harrism This PR is here for visibility - it's the code used by DALI. If you think it should be closed, it can be, but that's probably where I'll keep adjusting RMM to the proposed libcu++ memory_resource interfaces.

mzient avatar May 09 '21 18:05 mzient

This PR has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates. This PR will be labeled inactive-90d if there is no activity in the next 60 days.

github-actions[bot] avatar Nov 18 '21 18:11 github-actions[bot]

This PR has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates.

github-actions[bot] avatar Feb 16 '22 19:02 github-actions[bot]

Superseded by #1095. Closing.,

harrism avatar Nov 23 '22 06:11 harrism