Repository navigation
C2H cuda::buffer support - #11344
C2H cuda::buffer support#11344
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the
Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ➖ Normal Change: Feature Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
c2h/include/c2h/detail/checked_memory.cuh (1)
45-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion:
std::strtollaccepts negative and malformed input.C2H_DEVICE_MEMORY_LIMIT=-1casts to a near-SIZE_MAXlimit, which silently setsinfo.override = truewhile changing nothing. Reject values that do not parse to a positive number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6fa497b9-b4bb-4c98-8a0a-89132e3d0d2c
📒 Files selected for processing (16)
c2h/generators.cuc2h/generators_gen_values.cuc2h/generators_vector.cuc2h/include/c2h/buffer_generators.cuhc2h/include/c2h/catch2_test_helper.hc2h/include/c2h/checked_allocator.cuhc2h/include/c2h/checked_memory_resource.cuhc2h/include/c2h/detail/catch2_nvtx.hc2h/include/c2h/detail/catch2_seed.hc2h/include/c2h/detail/checked_memory.cuhc2h/include/c2h/detail/env.cuhc2h/include/c2h/detail/generators.cuhc2h/include/c2h/generator_common.hc2h/include/c2h/generators.hc2h/include/c2h/vector_generators.hcub/test/catch2_test_c2h_buffers.cu
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
85602af to
0611eea
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 669cbb1c-537b-4557-92c7-151ed2dceda5
📒 Files selected for processing (4)
c2h/generators.cuc2h/include/c2h/detail/checked_memory.cuhc2h/include/c2h/detail/generators.cuhcub/test/catch2_test_c2h_buffers.cu
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
0611eea to
be96c1c
Compare
This comment has been minimized.
This comment has been minimized.
|
The fix for NVHPC issue is filed in #11366 |
This comment has been minimized.
This comment has been minimized.
Simplified preprocessor guards accordingly
…ibution Add a multi-stream test that demonstrates race condition, confirm failure as per CodeRabbit review claim. Save generation/distribution per stream/device to resolve the race and fix the test.
…in prepare_random_generator(seed, num_items)
- Device-scoped cleanup remains the primary path. - If device selection throws, the state is reset without switching devices.
MSVC requires constexpr constants to be caputed, but Clang insists they should not be. Solution: do not use any_of, use count instead which does not need lambda to begin with.
This comment has been minimized.
This comment has been minimized.
Changes were requested at earlier stages of this work and have all been addressed.
bernhardmgruber
left a comment
There was a problem hiding this comment.
I reviewed everything except for the checked memory, checked resource, and tests.
gen_buffers used to create d_items using cuda::no_init, with `gen` later overwriting values. The issue arises for custom_type_t which contains pair of key/value and, possibly, additional bytes. The `gen` routine, however, sets key/value members only, leaving other members uninitialized. The reason tests did not catch it is that they use operator== to test content of the buffer which only compares key/value members. Initializing device buffer with user-provided type instance `min` ensures that all bytes are populated.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bernhardmgruber
left a comment
There was a problem hiding this comment.
Please consider the feedback above, otherwise LGTM
Rewrote condition to fail allocation if free memory is less than bytes + padding while stilla avoiding the possible integer overflow
All compiler in our support matrix support C++17
Also reused checked_host_allocate allocation from checked_allocator instead of reimplementing it.
instead of creating buffer with no_init followed by a call to copy_bytes
There was a problem hiding this comment.
🧹 Nitpick comments: 1
c2h/include/c2h/buffer_generators.cuh: 1
94-94: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winsuggestion: Skip the initial fill for ordinary values.
For ordinary
T,gen_values_betweenwrites every output element aftermake_device_bufferfills the full allocation withmin. Large buffers therefore incur an avoidable device pass. Usecuda::no_initwhen the generator writes the complete value. Keep theminfill forcustom_type_t, whose generator updates onlykeyandval. (raw.githubusercontent.com)
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: NVIDIA/cccl/.coderabbit.yaml
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
2b915da7-5b79-426d-ba8e-a4c9cd3cb453
📒 Files selected for processing: 4
c2h/include/c2h/buffer_generators.cuhc2h/include/c2h/checked_allocator.cuhc2h/include/c2h/detail/checked_memory.cuhcub/test/catch2_test_c2h_buffers.cu
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
Only initiaze buffer with `min` for custom_type_t.
This comment has been minimized.
This comment has been minimized.
🥳 CI Workflow Results🟩 Finished in 4h 23m: Pass: 100%/187 | Total: 8d 20h | Max: 3h 12m | Hits: 38%/551605See results here. |
Description
This PR contributes a portion of #10898
Adds a synchronous, device-accessible checked memory resource using
cudaMalloc/cudaFree, plusc2h::make_device_bufferfor creatingcuda::device_bufferinstances backed by it.Adds a synchronous, host-accessible memory resource backed by aligned new/delete. On integrated devices, it checks available device memory before allocating. Includes
c2h::make_host_bufferfor creatingcuda::host_bufferinstances backed by it.Adds stream-aware C2H generator support and the
gen_device_buffer,gen_buffers, andgen_host_bufferhelpers for generating data into checked CUDA buffers.Adds focused tests covering the resources, factories, allocation failures, and generated buffer contents.
closes #11343
gated by #11338Checklist