Repository navigation
[libcu++] Keep launch_transform storage alive until after the kernel launch on MSVC - #12138
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Priority: ➖ Normal Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
…launch on MSVC launch_transform kept the object returned by transform_launch_argument() in a by-value default argument and relied on it being destroyed after the kernel launch. Whether a by-value parameter is destroyed when the callee returns or at the end of the caller's full-expression is implementation-defined ([expr.call]/7). The Itanium ABI destroys it after the launch; the Microsoft ABI destroys it when launch_transform returns, before the kernel is enqueued. So post-launch actions, such as copying an output buffer back to the host, ran too early with MSVC. Bind the storage to an rvalue reference parameter instead. A temporary bound to a reference parameter persists until the end of the full-expression containing the call on every ABI.
bcc0abb to
9dcd7a2
Compare
|
/ok to test 9dcd7a2 |
This comment has been minimized.
This comment has been minimized.
| _Stream&& __stream, | ||
| _Arg&& __arg, | ||
| __optional_with_a_destructor<__transform_result_t<_Arg>> __storage = cuda::std::nullopt) const -> decltype(auto) | ||
| _Stream&& __stream, _Arg&& __arg, __optional_with_a_destructor<__transform_result_t<_Arg>>&& __storage = {}) const |
There was a problem hiding this comment.
This is wrong. This should be initialized with ::cuda::std::nullopt specifically, not {}, which may value-initialize depending on the chosen overload.
| _Stream&& __stream, _Arg&& __arg, __optional_with_a_destructor<__transform_result_t<_Arg>>&& __storage = {}) const | |
| _Stream&& __stream, _Arg&& __arg, __optional_with_a_destructor<__transform_result_t<_Arg>>&& __storage = ::cuda::std::nullopt) const |
There was a problem hiding this comment.
Done in 6881784, thanks. vector_add and the minimal repro still pass on Windows/MSVC (C++17 and C++20), and the libcu++ launch/stream/buffer and cudax tests pass on Linux.
|
/ok to test 6881784 |
This comment has been minimized.
This comment has been minimized.
|
Thanks for running CI. The only failing job was the CUB |
|
/ok to test 00a68a7 |
🥳 CI Workflow Results🟩 Finished in 2h 46m: Pass: 100%/228 | Total: 5d 05h | Max: 2h 11m | Hits: 73%/915285See results here. |
Description
Fixes the
vector_addfailure from #8902. Thestdexec_streamfailure there is a different bug, fixed in #12134.cuda::launch_transform(libcudacxx/include/cuda/__stream/launch_transform.h) keeps the object returned bytransform_launch_argument()in a by-value default argument:__optional_with_a_destructor<...> __storage = cuda::std::nullopt. The comment there expects this object to be destroyed in the caller, after the kernel launch. The standard leaves this implementation-defined ([expr.call]/7):launch_transformreturns, before the kernel is launched.So with MSVC every post-launch action runs before the launch. In
cudax/examples/vector.cuh,__action::~__actioncopiesoutbuffers back to the host. With MSVC it runs before the kernel, so the host copy ofCis stale, although the device result is correct.This PR binds the storage to an rvalue reference parameter instead:
__optional_with_a_destructor<...>&& __storage = {}. A temporary bound to a reference parameter persists until the end of the full-expression containing the call, on every ABI ([class.temporary]/6.9). The comments are updated to match.transform_launch_argument()is also implemented bycuda::buffer,cuda::uninitialized_async_buffer, thelogical_endpointtypes and the cudax buffers, so this is not limited to the example. The destructor check inlaunch_smoke.cuthat would catch this is commented out ("Disabled for now because we don't handle it with graphs").Testing
Plain C++ (
struct S { ~S(); }; int f(S s = S{}); g(f());): MSVC 19.51 runs~Sbeforeg, g++ 16.2 after. Withint f(S&& s = S{}), both run it afterg.Native Windows, RTX 5070 Ti (sm_120), driver 617.42, nvcc 13.4.92, MSVC 19.51, C++20:
kernel had run = 0, failscudax/examples/vector_add.cuTest PASSEDLinux (WSL2, nvcc 13.4.59, gcc 16.2), with this change:
libcudacxx.test.cuda.ccclrt.launch.*,ccclrt.stream.*,ccclrt.logical_endpoint: 8/8 pass.libcudacxx.test.cuda.containers.buffer.*,uninitialized_async_buffer,libcudacxx.test.debugging.buffer: 12/12 pass.cudax.test.{launch,stream,algorithm,containers,execution}and the examplesvector_add,async_buffer_add,stdexec_stream: 8/8 pass.cudax.test.executionandstdexec_streamneed [cudax] Do not read managed memory from the host after starting work in the stream adaptor #12134 as well; that fix is unrelated to this one.Not tested: clang-cl, nvc++.
Checklist
cudax/examples/vector_add.cuon MSVC. I did not re-enable the destructor check inlaunch_smoke.cu, because it is disabled for graph launches.)