[STF] Use out parameter for partition mappers#9117
Conversation
caugonnet
changed the title
|
/ok to test 5f33c27 |
|
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⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthroughimportant: Partitioner and executor function signatures are refactored from return-by-value to out-parameter convention. Type definitions are updated in CudaX and C wrapper headers, partition implementations are converted, call sites are adapted, test mappers are adjusted, and documentation is synchronized across the refactoring. ChangesPartitioner ABI migration
suggestion: Run full compile with nvcc and host toolchain and re-run unit tests that exercise partition get_executor paths to catch any ABI-callsite mismatches. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cudax/include/cuda/experimental/__places/partitions/blocked_partition.cuh (1)
105-123:⚠️ Potential issue | 🔴 Critical | ⚡ Quick wincritical:
blocked_partition_custom::get_executorcan divide by zero on Line 123 when the selected dimension extent is zero (part_sizebecomes 0). Add an early guard forgrid_dims.x == 0/part_size == 0before computingc / part_size.
🧹 Nitpick comments (2)
cudax/include/cuda/experimental/__places/localized_array.cuh (1)
362-364: ⚡ Quick winsuggestion: initialize
eplace_coordsbefore passing it tomapper(e.g.,pos4 eplace_coords(0);) so partially-written mapper outputs cannot leak uninitialized coordinates into placement decisions.cudax/include/cuda/experimental/__places/data_place_interface.cuh (1)
55-58: ⚡ Quick winsuggestion: Add explicit unstable API warning to the documentation comment. While the
cuda::experimentalnamespace indicates experimental status, the coding guidelines require that "All CUDA Experimental APIs must be documented as unstable and subject to change without notice." Consider adding a Doxygen@warningor note tag.//! Function type for computing executor placement from data coordinates. //! Uses an out-pointer convention so the signature is trivially representable //! in FFI frameworks (ctypes, cffi, Rust) that cannot return C structs. +//! `@warning` This API is experimental and subject to change without notice. using partition_fn_t = void (*)(pos4* result, pos4 data_coords, dim4 data_dims, dim4 grid_dims);As per coding guidelines: "All CUDA Experimental APIs must be documented as unstable and subject to change without notice".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 59d940a9-5232-4609-8a45-9d12e8f769af
📒 Files selected for processing (9)
c/experimental/stf/include/cccl/c/experimental/stf/stf.hc/experimental/stf/src/stf.cuc/experimental/stf/test/test_places.cppcudax/include/cuda/experimental/__places/data_place_interface.cuhcudax/include/cuda/experimental/__places/localized_array.cuhcudax/include/cuda/experimental/__places/partitions/blocked_partition.cuhcudax/include/cuda/experimental/__places/partitions/cyclic_shape.cuhcudax/include/cuda/experimental/__places/partitions/tiled_partition.cuhdocs/cudax/places.rst
This comment has been minimized.
This comment has been minimized.
|
/ok to test 4b1424e |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cudax/include/cuda/experimental/__places/localized_array.cuh (1)
362-362: ⚡ Quick winsuggestion: Use uniform initialization for
eplace_coords.Line 362 should use brace initialization to satisfy the header style rule:
pos4 eplace_coords{0};.As per coding guidelines: "Use uniform initialization for class constructors and compile-time conversions, e.g., constexpr auto x = int{sizeof(float)};".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 853a96b6-2a57-4d38-bee9-bb10c465a4f4
📒 Files selected for processing (1)
cudax/include/cuda/experimental/__places/localized_array.cuh
| //! Function type for computing executor placement from data coordinates. | ||
| //! Uses an out-pointer convention so the signature is trivially representable | ||
| //! in FFI frameworks (ctypes, cffi, Rust) that cannot return C structs. | ||
| using partition_fn_t = void (*)(pos4* result, pos4 data_coords, dim4 data_dims, dim4 grid_dims); |
There was a problem hiding this comment.
Can't we use here the typedef in c/experimental/stf/include/cccl/c/experimental/stf/stf.h so we don't need to duplicate it?
|
/ok to test ba9d290 |
This comment has been minimized.
This comment has been minimized.
|
/ok to test 71d8b77 |
| static const S_out apply(const S_in& in, pos4 position, dim4 grid_dims); | ||
|
|
||
| pos4 get_executor(pos4 data_coords, dim4 data_dims, dim4 grid_dims); | ||
| void get_executor(pos4* result, pos4 data_coords, dim4 data_dims, dim4 grid_dims); |
There was a problem hiding this comment.
Suggestion: should this be static void get_executor(...)? partition_fn_t is a plain function pointer type not a member function. Also the text below mentions a virtual method, which this is not
This comment has been minimized.
This comment has been minimized.
|
/ok to test 23091ba |
🥳 CI Workflow Results🟩 Finished in 36m 57s: Pass: 100%/59 | Total: 5h 12m | Max: 31m 09s | Hits: 100%/33226See results here. |
* [CUDAX] Use out parameter for partition mappers * [CUDAX] Initialize mapper output coordinates * Apply suggestion from @andralex * Update cudax/include/cuda/experimental/__places/partitions/cyclic_shape.cuh --------- Co-authored-by: Andrei Alexandrescu <andrei@erdani.com>
3 participants
Summary
partition_fn_t/stf_get_executor_fnto write mapper results through an out parameter instead of returningpos4by value.stf_c_apibranch work.Test plan
pre-commit run --files c/experimental/stf/include/cccl/c/experimental/stf/stf.h c/experimental/stf/src/stf.cu c/experimental/stf/test/test_places.cpp cudax/include/cuda/experimental/__places/data_place_interface.cuh cudax/include/cuda/experimental/__places/localized_array.cuh cudax/include/cuda/experimental/__places/partitions/blocked_partition.cuh cudax/include/cuda/experimental/__places/partitions/cyclic_shape.cuh cudax/include/cuda/experimental/__places/partitions/tiled_partition.cuh docs/cudax/places.rstgit diff --check