Only enabled PDL for PTX/SASS supporting it#9163
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:
📝 Walkthroughimportant: Two launcher factories add __assert_pdl_allowed checks that assert dependent_launch is permitted only on SM90+; multiple dispatch paths now pass dependent_launch conditionally (ptx_version >= 900 or cc >= 9.0) instead of unconditionally. PDL Support Validation and Dispatch Threading
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cub/cub/detail/launcher/cuda_runtime.cuh (1)
33-33: ⚡ Quick winsuggestion: Provide a descriptive assertion message for the PtxComputeCap query failure.
The empty string
""doesn't communicate why the assertion failed. A message like"Failed to query PTX compute capability"would aid debugging.-_CCCL_ASSERT(PtxComputeCap(cc) == cudaSuccess, ""); +_CCCL_ASSERT(PtxComputeCap(cc) == cudaSuccess, "Failed to query PTX compute capability");
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ba4ba4ae-9074-481b-8696-03db2f9f78a4
📒 Files selected for processing (2)
cub/cub/detail/launcher/cuda_driver.cuhcub/cub/detail/launcher/cuda_runtime.cuh
|
Actionable comments posted: 0 |
2 similar comments
|
Actionable comments posted: 0 |
|
Actionable comments posted: 0 |
This comment has been minimized.
This comment has been minimized.
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: 6bb42824-b1a0-4633-9292-2594ddebd58a
📒 Files selected for processing (3)
cub/cub/device/dispatch/dispatch_merge_sort.cuhcub/cub/device/dispatch/dispatch_scan.cuhcub/cub/device/dispatch/dispatch_transform.cuh
|
Let's see how far back we can backport this. |
This comment has been minimized.
This comment has been minimized.
| active_policy.threads_per_block, | ||
| 0, | ||
| stream, | ||
| /* dependent_launch */ cc >= ::cuda::compute_capability{9, 0}) |
There was a problem hiding this comment.
Can we rather introduce:
enum class enable_dependent_launch : bool {};and do:
| /* dependent_launch */ cc >= ::cuda::compute_capability{9, 0}) | |
| enable_dependent_launch{cc >= ::cuda::compute_capability{9, 0}}) |
I hate that we need the comment
There was a problem hiding this comment.
I just tried that and found it a bit cumbersome. Where should we put the definition of this enum? It's needed in Thrust and CUB.
Co-authored-by: David Bayer <48736217+davebayer@users.noreply.github.com>
Co-authored-by: David Bayer <48736217+davebayer@users.noreply.github.com>
🥳 CI Workflow Results🟩 Finished in 2h 27m: Pass: 100%/285 | Total: 11d 18h | Max: 2h 27m | Hits: 13%/1266121See results here. |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin branch/3.3.x
git worktree add -d .worktree/backport-9163-to-branch/3.3.x origin/branch/3.3.x
cd .worktree/backport-9163-to-branch/3.3.x
git switch --create backport-9163-to-branch/3.3.x
git cherry-pick -x 52931dd9a79f2159c8c30b7852898636f8d17f0f |
|
Successfully created backport PR for |
|
Manual backport to 3.3: #9188 |
Fixes: NVIDIA#9134 Co-authored-by: David Bayer <48736217+davebayer@users.noreply.github.com>
The changes in this PR assert that PDL is only enabled with PTX/SASS supporting it. The assertions are not triggered by any CI run, but I can hit them locally by e.g. compiling for SM80 and running on SM120 (e.g.
cub.test.device.transform.lid_0).Then, a fix is proposed by guarding any PDL use by the compute capability of the PTX used to launch.
Fixes: #9134