[libcu++] Add new memory pool attributes from CUDA 13.3#9798
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughCUDA 13.3 memory-pool creation attributes now use typed driver queries, are exposed through ChangesMemory pool attribute support
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
libcudacxx/test/libcudacxx/cuda/memory_resource/resources/device_memory_resource.cu (1)
71-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Const-qualify and annotate the new helpers.
Lines 73 and 80 only read their parameters. Add
constqualification, markget_raw_attribute[[nodiscard]], and declare the non-template helperinline.As per coding guidelines, unchanged variables must be const, non-template non-constexpr functions must be inline, and most non-void results should be
[[nodiscard]].Source: Coding guidelines
libcudacxx/test/libcudacxx/cuda/memory_resource/resources/memory_pools.cu (1)
127-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Make the creation-attribute checker const-correct.
check_creation_attributesonly readspool,props, andcurrent_device. Useconst PoolType&,const cuda::memory_pool_properties, andconst intfor these parameters.As per coding guidelines, variables that are not modified must be declared const.
Source: Coding guidelines
libcudacxx/test/libcudacxx/cuda/memory_resource/resources/shared_memory_pools.cu (1)
51-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Const-qualify the checker inputs.
check_shared_pool_creation_attributesonly readspoolandprops. Useconst PoolType&andconst cuda::memory_pool_properties.As per coding guidelines, variables that are not modified must be declared const.
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6e27e8f7-12cc-479b-8ebc-6705643498de
📒 Files selected for processing (6)
docs/libcudacxx/runtime/memory_pools.rstlibcudacxx/include/cuda/__driver/driver_api.hlibcudacxx/include/cuda/__memory_pool/memory_pool_base.hlibcudacxx/test/libcudacxx/cuda/memory_resource/resources/device_memory_resource.culibcudacxx/test/libcudacxx/cuda/memory_resource/resources/memory_pools.culibcudacxx/test/libcudacxx/cuda/memory_resource/resources/shared_memory_pools.cu
| } | ||
| # endif // _CCCL_CTK_AT_LEAST(13, 0) | ||
|
|
||
| CHECK(pool.attribute(cuda::memory_pool_attributes::allocation_type) == expected_allocation_type); |
There was a problem hiding this comment.
Nit: prefer REQUIRE(), is there any point in continuing the test if this fails?
| CHECK_THROWS_AS( | ||
| pool.set_attribute( | ||
| cuda::memory_pool_attributes::allocation_type, pool.attribute(cuda::memory_pool_attributes::allocation_type)), | ||
| std::invalid_argument); |
There was a problem hiding this comment.
Important: assert the exception message as well to make sure you are triggering exactly the exception you think you're triggering.
| } | ||
|
|
||
| #if _CCCL_CTK_AT_LEAST(13, 3) && _CCCL_HAS_EXCEPTIONS() | ||
| { // CUDA 13.3 creation attributes |
There was a problem hiding this comment.
Nit: does it need to be inside a scope?
| ::cuda::std::size_t __value = 0; | ||
| static auto __driver_fn = _CCCLRT_GET_DRIVER_FUNCTION(cuMemPoolGetAttribute); | ||
| _Tp __value{}; | ||
| static auto __driver_fn = _CCCLRT_GET_DRIVER_FUNCTION(cuMemPoolGetAttribute); |
There was a problem hiding this comment.
Important: the driver function will be loaded for every instantiation of this function. Consider having a type-less stub called by the templates to avoid reloading it multiple times (probably also slightly better for compile-time since the compilers don't have to generate all that code every time).
| template <::cudaMemPoolAttr _Attr, | ||
| typename _Type, | ||
| __pool_attr_settable _Settable, | ||
| typename _StorageType = _Type> |
There was a problem hiding this comment.
IMO, if this was such a problem that we need to specify this, then we shouldn't make it a default. Even if it's verbose for a lot of cases, it forces us to consider the type every time we add a new one.
| [[nodiscard]] _CCCL_HOST_API type operator()(::cudaMemPool_t __pool) const | ||
| { | ||
| size_t __value = ::cuda::__driver::__mempoolGetAttribute(__pool, static_cast<::CUmemPool_attribute>(_Attr)); | ||
| auto __value = ::cuda::__driver::__mempoolGetAttribute<_StorageType>( |
There was a problem hiding this comment.
| auto __value = ::cuda::__driver::__mempoolGetAttribute<_StorageType>( | |
| const auto __value = ::cuda::__driver::__mempoolGetAttribute<_StorageType>( |
(nit, but if you agree, elsewhere as well pls)
| _CCCL_THROW(::std::invalid_argument, "This attribute can't be set to a non-zero value."); | ||
| } | ||
| ::cuda::__driver::__mempoolSetAttribute(__pool, __attr, &__value); | ||
| ::cuuint64_t __value_copy = static_cast<::cuuint64_t>(__value); |
There was a problem hiding this comment.
Maybe just make __value a cuuint64_t directly? This is only called by us, and we probably want to know where we're using the wrong type.
pciolkosz
force-pushed
the
cuda133-memory-pool-attrs
branch
from
5d8c4ce to
8880974
Compare
pciolkosz
force-pushed
the
cuda133-memory-pool-attrs
branch
from
8880974 to
afa9214
Compare
This comment has been minimized.
This comment has been minimized.
🥳 CI Workflow Results🟩 Finished in 2h 31m: Pass: 100%/120 | Total: 2d 22h | Max: 2h 31m | Hits: 69%/524903See results here. |
3 participants
CUDA 13.3 added a bunch of read-only memory pool attributes. This PR adds support for it for the CCCL memory pool types.
Also includes a fix to make the attribute getter correctly use the type specified in the documentation