Repository navigation
Use the new tuning API for detail::radix_sort::dispatch - #7949
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
DeviceRadixSortdetail::radix_sort::dispatch
e24e2be to
c23f8b8
Compare
This comment has been minimized.
This comment has been minimized.
a4e3081 to
dcbf736
Compare
d77e959 to
43a6707
Compare
43a6707 to
db584e2
Compare
db584e2 to
dc279a8
Compare
dc279a8 to
ea0103c
Compare
01e69f3 to
d959589
Compare
|
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)
📝 WalkthroughSummary by CodeRabbit
important: ## Walkthrough DeviceRadixSort dispatch was centralized into select_tuning_and_dispatch and tuning_env is threaded through decomposer and public SortKeys/SortPairs overloads; benchmarks now call DeviceRadixSort directly with env-backed allocator and updated nvbench metrics; tests were added to measure tuning-driven temporary-storage allocations. important: ## Changes Radix Sort tuning API integration and dispatch routing
important: ## Assessment against linked issues
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cub/cub/device/device_radix_sort.cuh (1)
191-224:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd TuningEnvT parameter to
radix_sort_with_decomposerand pass it toselect_tuning_and_dispatch. Currently, env-basedSortPairs/SortKeysoverloads for custom keys extract tuning from the environment but drop it on the floor becauseradix_sort_with_decomposerhas noTuningEnvTparameter. This breakscuda::execution::tune(...)for decomposer-based env APIs. Either add the parameter to thread tuning through, or switch env-based decomposer overloads to usedispatch_with_env_and_tuninglike other algorithms (e.g., adjacent_difference).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fa87ecb6-ce9c-4097-9f0f-5f180acf4934
📒 Files selected for processing (5)
cub/benchmarks/bench/radix_sort/keys.cucub/benchmarks/bench/radix_sort/pairs.cucub/benchmarks/bench/radix_sort/policy_selector.hcub/cub/device/device_radix_sort.cuhcub/test/catch2_test_device_radix_sort_env.cu
| auto l = [&](auto env) { | ||
| auto data = c2h::device_vector<int>(10'000); // must be larger than the single tile path | ||
| return cub::DeviceRadixSort::SortPairs( | ||
| data.data().get(), | ||
| data.data().get(), | ||
| data.data().get(), | ||
| data.data().get(), | ||
| static_cast<int>(data.size()), | ||
| 0, | ||
| 32, | ||
| env); |
There was a problem hiding this comment.
important: these tuning tests are invoking unsupported in-place radix-sort variants. The pointer cases alias *_in and *_out, and the DoubleBuffer cases use the same pointer for current and alternate storage, so the allocation-size assertions are currently riding on undefined behavior instead of a valid dispatch path. Use distinct input/output buffers here even if the test only cares about temporary-storage size.
Also applies to: 1168-1172, 1181-1192, 1201-1205, 1214-1218, 1227-1231, 1240-1244, 1253-1257
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cub/test/catch2_test_device_radix_sort_env.cu (1)
1154-1164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winimportant: These tuning tests still exercise unsupported in-place radix-sort paths by aliasing input/output buffers and by using
DoubleBufferwith identical current/alternate pointers. Examples include Line 1157-Line 1160, Line 1176, Line 1235, Line 1410-Line 1413, and Line 1466-Line 1469. This can make temp-storage-size assertions depend on undefined behavior instead of a valid dispatch path. Use distinct input/output buffers for pointer overloads, distinct current/alternate buffers for eachDoubleBuffer, and separate key/value storage for pairs.As per coding guidelines:
cub/**/*: focus on algorithm correctness and temporary-storage protocol.Also applies to: 1174-1178, 1187-1198, 1207-1211, 1220-1224, 1233-1237, 1246-1250, 1259-1263, 1274-1278, 1287-1291, 1330-1334, 1343-1347, 1405-1415, 1462-1474, 1484-1494
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a6307174-f001-4fc0-a1a7-6a2821c0d302
📒 Files selected for processing (2)
cub/cub/device/device_radix_sort.cuhcub/test/catch2_test_device_radix_sort_env.cu
🚧 Files skipped from review as they are similar to previous changes (1)
- cub/cub/device/device_radix_sort.cuh
This comment has been minimized.
This comment has been minimized.
8b996b2 to
c019054
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 65f2b8ec-e67b-4638-8c7c-b53df36b2f05
📒 Files selected for processing (5)
cub/benchmarks/bench/radix_sort/keys.cucub/benchmarks/bench/radix_sort/pairs.cucub/benchmarks/bench/radix_sort/policy_selector.hcub/cub/device/device_radix_sort.cuhcub/test/catch2_test_device_radix_sort_env.cu
🚧 Files skipped from review as they are similar to previous changes (4)
- cub/benchmarks/bench/radix_sort/pairs.cu
- cub/benchmarks/bench/radix_sort/keys.cu
- cub/test/catch2_test_device_radix_sort_env.cu
- cub/cub/device/device_radix_sort.cuh
🥳 CI Workflow Results🟩 Finished in 1h 40m: Pass: 100%/283 | Total: 4d 00h | Max: 1h 38m | Hits: 98%/199284See results here. |
cub.bench.radix_sort.keys.baseon SM75;80;90;100cub.bench.radix_sort.pairs.baseon SM75;80;90;100Fixes: #7947