Skip to content

fix(gpu): copy get/set elements in-kernel instead of device-side cudaMemcpyAsync - #1230

Open
pcchen wants to merge 4 commits into
masterfrom
fix/1229-gpu-getset-device-memcpy
Open

pcchen wants to merge 4 commits into
masterfrom
fix/1229-gpu-getset-device-memcpy

Conversation

@pcchen

@pcchen pcchen commented Nov 12, 2026 •

Copy link
Copy Markdown
Collaborator

Posted by Claude Code on behalf of @pcchen

Fixes #1229.

Problem

cuGetElems_contiguous_kernel and the (cd, cd) and (cf, cf) overloads of cuSetElems_conti_gpu_kernel copied each selected unit with a device-side cudaMemcpyAsync, which is one CUDA dynamic-parallelism child launch per kernel thread, and discarded its return value. The device runtime holds at most cudaLimitDevRuntimePendingLaunchCount (default 2048) pending launches. A GPU Tensor::get, or a complex Tensor::set, selecting more units than that therefore:

  • silently left a random subset of the elements uncopied, and
  • occasionally never returned (host spinning in cudaFree), which is the intermittent hang of linalg_Test.GpuBkUtSvdTruncate1/2.

Details and the Cytnx-free reproducer results are in #1229.

Fix

Copy the unit with a plain loop inside the kernel, as the other cuSetElems_conti_gpu_kernel overloads already do. No element selection or arithmetic changes; only how the bytes are moved. After this change nothing in src/ uses device-side launches.

Two regression tests in tests/gpu/Tensor_test.cpp (TensorGetSetManyElements.*): GPU get and complex GPU set of 30000 single-element units, checked against independently computed values.

Testing

GPU builds with -DUSE_CUTENSOR=OFF -DUSE_CUQUANTUM=OFF. "Unpatched" is the same tree with the two kernel files reverted.

Tesla V100, CUDA 12.9:

unpatched patched
new tests fail, about 27900 of 30000 elements wrong (97 of 100 runs; the other 3 hang) pass
GpuBkUtSvdTruncate* looped as fresh processes 31 hangs in 800 runs 0 hangs in 600 runs
full gpu_test_main (at eade1422 plus this change) 834 pass, 11 skip, 2 fail (the new tests); SVD-truncate tests excluded so the run cannot hang 849 run: 838 pass, 11 skip, 0 fail
first commit on master f9e1027: new tests fail (27873 and 27865 of 30000 wrong) pass
first commit on master f9e1027: GpuBkUtSvdTruncate* plus new tests, looped 200 times 4 hangs (SVD tests only) 0 hangs
first commit on master f9e1027: full gpu_test_main not run 847 run: 835 pass, 12 skip, 0 fail; 2 further tests excluded, see below
branch head after merging master c7fda0a (8de452f): full gpu_test_main not run 852 run: 841 pass, 11 skip, 0 fail; nothing excluded

The 2 tests excluded from the full run at f9e1027 are GesvdTruncate.GpuFlagCombinations and Rsvd.GpuFlagCombinations. At that base each ended the test process with CUSOLVER_STATUS_EXECUTION_FAILED from cusolverDnDgesvdj, with or without this change. That was #1102, fixed by #1228, which has since merged: at the current branch head both tests pass on the V100 (5 of 5 runs each) and nothing is excluded from the full run.

GeForce MX450 2 GB (sm_75), CUDA 13.2, first commit on f9e1027, 95 Tensor/Accessor/SVD/linalg tests run one process each:

unpatched patched
new tests fail (27947 and 27344 of 30000 wrong) pass
GpuBkUtSvdTruncate* looped 100 times 2 hangs 0 hangs
per-test result 69 pass, 11 skip, 13 out of GPU memory (2 GB card), 2 fail

The 2 failures on the MX450 are the same two gesvdj tests (#1102). With master c7fda0a they pass there too (3 of 3 runs each, checked with the get/set kernels unpatched).

The second commit (std::uint64_t loop counters, from review) was rebuilt on the V100 only: 33 Tensor/Accessor get/set tests pass and 60 looped SVD-truncate runs gave 0 hangs.

Not tested: a Cytnx build on the A100 (only the standalone reproducer in #1229 was run there), and performance of GPU get/set; no speed claim is made. CPU builds do not compile the changed files.

pre-commit (clang-format v14) passes on the changed files.

🤖 Generated with Claude Code

…MemcpyAsync

cuGetElems_contiguous_kernel and the (cd, cd) and (cf, cf) overloads of
cuSetElems_conti_gpu_kernel copied each unit with a device-side
cudaMemcpyAsync, one dynamic-parallelism child launch per kernel thread,
and discarded its return value. The device runtime holds at most
cudaLimitDevRuntimePendingLaunchCount (default 2048) pending launches, so
a call selecting more units silently left a random subset uncopied and
occasionally never returned.

Copy the unit with a plain loop, as the other cuSetElems overloads
already do. Add GPU regression tests for get and complex set of 30000
single-element units.

Fixes #1229

Co-Authored-By: Claude <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-12T23:42:27.748566Z 201aa14 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 201aa14c0b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@codecov

codecov Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.43%. Comparing base (c7fda0a) to head (0199571).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1230   +/-   ##
=======================================
  Coverage   69.43%   69.43%           
=======================================
  Files         209      209           
  Lines       22614    22614           
  Branches       72       72           
=======================================
  Hits        15701    15701           
  Misses       6891     6891           
  Partials       22       22           
Flag Coverage Δ
cpp 69.50% <ø> (ø)
python 65.01% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
C++ backend 69.50% <ø> (ø)
Python bindings ∅ <ø> (∅)
Python package 65.01% <ø> (ø)

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7fda0a...0199571. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The counters added for #1229 are plain loop indices, not dtype-backed
values, so they take a standard type per AGENTS.md. Neighbouring
pre-existing loops are left as they are.

Co-Authored-By: Claude <noreply@anthropic.com>
@pcchen
pcchen requested a review from IvanaGyro November 13, 2026 00:01
@pcchen

pcchen commented Nov 13, 2026

Copy link
Copy Markdown
Collaborator Author

@ianmccul Pleaase help to review tihs.

@ianmccul

Copy link
Copy Markdown
Collaborator

The fix looks correct. One device-side cudaMemcpyAsync per thread was never a good design. These kernels are still slow: one thread per unit, uncoalesced when units are larger than one element, and four allocations plus synchronous copies per call. But that's true of the GPU get/set path in general and not this PR's job. I wouldn't tune these two kernels in isolation.

Two cheap follow-ups:

  1. Now that nothing in src/ uses dynamic parallelism, check whether CUDA_SEPARABLE_COMPILATION / CUDA_RESOLVE_DEVICE_SYMBOLS and the cudadevrt link are still needed. If they're only there for this, dropping them should shorten link time and allow cross-TU optimisation.
  2. Add a line to the AGENTS.md domain gotchas: "No device-side kernel launches or cudaMemcpy* from device code (dynamic parallelism); copy in-kernel." This code was a line-by-line port of a CPU memcpy. That's exactly the mistake an agent would repeat, and the rule costs nothing.

Add a domain gotcha recording the cause of #1229, as suggested in review:
the get/set kernels were a line-by-line port of a CPU memcpy, a mistake
that is easy to repeat.

Co-Authored-By: Claude <noreply@anthropic.com>
@pcchen

pcchen commented Nov 13, 2026

Copy link
Copy Markdown
Collaborator Author

Posted by Claude Code on behalf of @pcchen

Thanks for the review.

  1. Added to the AGENTS.md domain gotchas in 0199571.
  2. Agreed, and left out of this PR: dropping CUDA_SEPARABLE_COMPILATION / CUDA_RESOLVE_DEVICE_SYMBOLS and the cudadevrt link changes the build system and needs a full GPU rebuild and suite run to verify, plus a measurement before any link-time claim. It will go in a separate PR.

Agreed as well that the general GPU get/set path cost is out of scope here.

@IvanaGyro

IvanaGyro commented Nov 13, 2026 •

Copy link
Copy Markdown
Member
  1. Now that nothing in src/ uses dynamic parallelism, check whether CUDA_SEPARABLE_COMPILATION / CUDA_RESOLVE_DEVICE_SYMBOLS and the cudadevrt link are still needed. If they're only there for this, dropping them should shorten link time and allow cross-TU optimisation.

We enable LTO now, so those two flag may need to be enabled to generate SASS for multiple sm.

I have forgot the context. However, #1171 mentions these two flags.

@pcchen for verifying this before making the change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[gpu] Tensor get/set silently drops elements (and occasionally hangs) for selections over 2048 elements

3 participants