-
Notifications
You must be signed in to change notification settings - Fork 287
[cudax->libcudacxx] Move device_transform to libcu++ #6469
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[cudax->libcudacxx] Move device_transform to libcu++ #6469
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| template <typename _Tp> | ||
| struct __optional_with_a_destructor : ::cuda::std::optional<_Tp> | ||
| { | ||
| using ::cuda::std::optional<_Tp>::optional; | ||
| ~__optional_with_a_destructor() {} | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand why this needed, the destructor will still run at the same time, its just that there is nothing to destroy there
|
|
||
| namespace __tfx | ||
| { | ||
| // Device transform: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design question: is device transform a good name for this object? Whenever I see the name, I am immediately thinking about some cub algorithm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It started as launch_transform because it was used for cuda::launch API. Later we added it for copy_bytes and I believe some things Eric is working on. In general the idea was anytime execution is transferred to the device, arguments passed are going to this transformation. This is how device_transform name came up. Would device_argument_transform help with this concern? I can't think of a fundamentally different name for this
🥳 CI Workflow Results🟩 Finished in 2h 46m: Pass: 100%/122 | Total: 1d 18h | Max: 2h 01m | Hits: 82%/236714See results here. |
This PR moves
device_transformfrom cudax to libcu++. I had to adjust some things it used from cudax. One of them wascuda::experimental::__variant, which I replaced withcuda::std::optional. I believe it was just a simpler type we can later go back to.With
device_transformmoved to libcu++ there was no longer a need to keep the experimental version ofcopy_bytesandfill_bytes, I removed them and added device transform to the non-experimental version.I had to still leave experimental tests of the
algorithmheader, because ofuninitialized_bufferused in them. We can move them when we move that type.