-
Notifications
You must be signed in to change notification settings - Fork 290
[CUB] Implement BlockLoadToShared
#5780
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
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. |
| //! @{ | ||
|
|
||
| //! @brief Collective constructor using a private static allocation of shared memory as temporary storage. | ||
| _CCCL_DEVICE _CCCL_FORCEINLINE BlockLoadToShared() |
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 believe those should just be _CCCL_DEVICE_API
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.
That's not what I see for other block-wide primitives, but if this is the new guideline, sure! Do I understand correctly that I should avoid _CCCL_FORCEINLINE?
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.
_CCCL_DEVICE_API is missing from https://nvidia.github.io/cccl/cccl/development/macro.html#visibility-macros. It would be great if any guidelines regarding usage of visibility macros and _CCCL_FORCEINLINE could be codified in https://github.com/NVIDIA/cccl/wiki/Cpp-Coding-Guidelines#general.
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.
Regarding visibility are you referring to the following?
Defaulted constructors should be marked with
_CCCL_HIDE_FROM_ABI
Currently it is listed as a libcu++-specific guideline. Should it rather be a general guideline? And to me "defaulted" means a constructor with = default and not a default-constructor. Is the formulation wrong?
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.
@bernhardmgruber Opinions?
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.
Should use _CCCL_DEVICE_API
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.
For all members or just constructors? No _CCCL_FORCEINLINE or both? I can add it in another PR.
1f44d1a to
9427acc
Compare
bernhardmgruber
left a comment
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.
Good work so far! Let's add some unit test to cover the API and show how it's used.
751e4ad to
0dae021
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
0dae021 to
fdf6ccf
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Stop invalidating in destructor
This reverts commit 5614a98.
7567f2d to
aaa2168
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
🥳 CI Workflow Results🟩 Finished in 4h 50m: Pass: 100%/185 | Total: 1d 19h | Max: 1h 18m | Hits: 99%/186891See results here. |
Description
closes #5693
Checklist