Fix CCCL main compatibility: explicit alignment and NVCC host/device diagnostics#2343
Conversation
…diagnostics CCCL main (3.4.0) deprecates default-alignment overloads of allocate_sync/deallocate_sync/allocate/deallocate on resource_ref types, promoting the deprecation warning to an error under -Werror. Additionally, basic_resource_ref's copy constructor is __host__-only, which causes NVCC error 20011 when thrust_allocator's implicitly-generated copy constructor (inherited as __host__ __device__ from thrust::device_malloc_allocator) attempts to copy the cccl_async_resource_ref member. cccl_adaptors.hpp: Pass rmm::CUDA_ALLOCATION_ALIGNMENT explicitly to all forwarded allocate/deallocate calls on the underlying resource_ref, avoiding the deprecated default-alignment codepath. thrust_allocator_adaptor.hpp: Provide explicit copy/move constructors with RMM_EXEC_CHECK_DISABLE so the pragma has a function body to attach to (it has no effect on = default). This mirrors the pattern used for device_uvector's destructor and move constructor. resource_ref_conversion_tests.cpp: Pass explicit alignment to allocate_sync/deallocate_sync on the type-erased synchronous_resource_ref to avoid the same deprecation error.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR updates memory allocation and deallocation methods across RMM wrapper classes to explicitly pass a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/include/rmm/mr/thrust_allocator_adaptor.hpp (1)
9-17: Add explicit#include <utility>forstd::movein public header.This header uses
std::movein the move constructor (lines 104, 106) but does not directly include<utility>. While transitive includes may mask this dependency, public API headers should explicitly include all headers for functionality they use.♻️ Suggested fix
`#include` <rmm/detail/exec_check_disable.hpp> `#include` <rmm/detail/export.hpp> `#include` <rmm/detail/thrust_namespace.h> `#include` <rmm/mr/per_device_resource.hpp> `#include` <rmm/resource_ref.hpp> `#include` <thrust/device_malloc_allocator.h> `#include` <thrust/device_ptr.h> `#include` <thrust/memory.h> +#include <utility>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/include/rmm/mr/thrust_allocator_adaptor.hpp` around lines 9 - 17, This header uses std::move in the move constructor of thrust_allocator_adaptor but doesn't include <utility>; add an explicit `#include` <utility> to the top of cpp/include/rmm/mr/thrust_allocator_adaptor.hpp so the public header directly provides the declaration for std::move (ensuring the move constructor code in thrust_allocator_adaptor compiles even without transitive includes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/include/rmm/mr/thrust_allocator_adaptor.hpp`:
- Around line 122-125: The templated rebind constructor
thrust_allocator(thrust_allocator<U> const& other) incorrectly initializes _mr
from other.resource() instead of copying the allocator's underlying PMR pointer;
change the initialization to copy other._mr (or call
other.get_upstream_resource() / other._mr) so the rebound allocator preserves
the exact upstream RMM resource used by non-templated constructors and avoid
drifting to a different resource; update the initializer list for
thrust_allocator<U> to use other._mr (or other.get_upstream_resource()) while
keeping stream() and _device as-is.
---
Nitpick comments:
In `@cpp/include/rmm/mr/thrust_allocator_adaptor.hpp`:
- Around line 9-17: This header uses std::move in the move constructor of
thrust_allocator_adaptor but doesn't include <utility>; add an explicit `#include`
<utility> to the top of cpp/include/rmm/mr/thrust_allocator_adaptor.hpp so the
public header directly provides the declaration for std::move (ensuring the move
constructor code in thrust_allocator_adaptor compiles even without transitive
includes).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc5ecd2c-5d99-48ec-9880-4c1111d4b3a7
📒 Files selected for processing (3)
cpp/include/rmm/detail/cccl_adaptors.hppcpp/include/rmm/mr/thrust_allocator_adaptor.hppcpp/tests/mr/resource_ref_conversion_tests.cpp
| RMM_EXEC_CHECK_DISABLE | ||
| template <typename U> | ||
| thrust_allocator(thrust_allocator<U> const& other) | ||
| : _mr(other.resource()), _stream{other.stream()}, _device{other._device} |
There was a problem hiding this comment.
Rebind construction should copy the allocator’s _mr, not resource().
Line 125 initializes _mr from other.resource(), but this class’s actual upstream RMM state is _mr/get_upstream_resource(). The non-templated constructors only populate _mr, so rebinding from thrust_allocator<U> can drift away from the original RMM resource here.
🛠️ Proposed fix
template <typename U>
thrust_allocator(thrust_allocator<U> const& other)
- : _mr(other.resource()), _stream{other.stream()}, _device{other._device}
+ : _mr(other.get_upstream_resource()), _stream{other.stream()}, _device{other._device}
{
}Based on learnings: Memory resources must maintain PMR (polymorphic memory resource) compatibility; follow standard PMR interface contracts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RMM_EXEC_CHECK_DISABLE | |
| template <typename U> | |
| thrust_allocator(thrust_allocator<U> const& other) | |
| : _mr(other.resource()), _stream{other.stream()}, _device{other._device} | |
| RMM_EXEC_CHECK_DISABLE | |
| template <typename U> | |
| thrust_allocator(thrust_allocator<U> const& other) | |
| : _mr(other.get_upstream_resource()), _stream{other.stream()}, _device{other._device} | |
| { | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/include/rmm/mr/thrust_allocator_adaptor.hpp` around lines 122 - 125, The
templated rebind constructor thrust_allocator(thrust_allocator<U> const& other)
incorrectly initializes _mr from other.resource() instead of copying the
allocator's underlying PMR pointer; change the initialization to copy other._mr
(or call other.get_upstream_resource() / other._mr) so the rebound allocator
preserves the exact upstream RMM resource used by non-templated constructors and
avoid drifting to a different resource; update the initializer list for
thrust_allocator<U> to use other._mr (or other.get_upstream_resource()) while
keeping stream() and _device as-is.
|
/merge |
Description
CCCL main (3.4.0) introduces two breaking changes that affect RMM:
Deprecated default-alignment overloads:
allocate_sync,deallocate_sync,allocate, anddeallocateonresource_reftypes no longer provide a default alignment parameter. Under-Werror, the deprecation warning becomes a build failure.basic_resource_refcopy constructor is__host__-only:thrust_allocatorinherits__host__ __device__fromthrust::device_malloc_allocator, so its implicitly-generated copy constructor calls the__host__-onlycccl_async_resource_refcopy constructor, triggering NVCC error 20011.Changes
cpp/include/rmm/detail/cccl_adaptors.hpp: Passrmm::CUDA_ALLOCATION_ALIGNMENTexplicitly to all forwardedallocate/deallocate/allocate_sync/deallocate_synccalls on the underlyingresource_ref, avoiding the deprecated default-alignment codepath.cpp/include/rmm/mr/thrust_allocator_adaptor.hpp: Provide explicit copy/move constructors withRMM_EXEC_CHECK_DISABLEso the#pragma nv_exec_check_disablehas a function body to attach to (it has no effect on= default). This mirrors the pattern used fordevice_uvector's destructor and move constructor.cpp/tests/mr/resource_ref_conversion_tests.cpp: Pass explicit alignment toallocate_sync/deallocate_syncon the type-erasedsynchronous_resource_refto avoid the same deprecation error.Validated by building with CCCL main (clean 119/119 targets including tests and benchmarks).
Checklist