Skip to content
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

add trait IsKernelArgumentTriviallyCopyable #2198

Conversation

psychocoderHPC
Copy link
Member

fix #2195

Provide an alpaka specific trait to validate kernel argument conditions which can be specialized by the user for their own types and at their own risk.

///
/// @tparam T type to check
/// @{
template<typename T>

Choose a reason for hiding this comment

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

Adding an additional template parameter typename = void makes specialization usually simpler.

Suggested change
template<typename T>
template<typename T, typename = void>

Instead of creating a specialization for every type, it can be generalized like:

template<typename Real>
struct IsKernelArgumentTriviallyCopyable<Real, typename std::enable_if<codi::isForwardType<Real>::value>::type

Copy link
Contributor

Choose a reason for hiding this comment

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

@MaxSagebaum thanks for the suggestion, I think it makes sense

Copy link
Member

Choose a reason for hiding this comment

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

You guys are going to love requires clauses in C++20.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point I changed the template signature

};

template<typename T>
constexpr bool IsKernelArgumentTriviallyCopyable_v = IsKernelArgumentTriviallyCopyable<T>::value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Following the styles of the other alpaka traits (like isQueue<T>, etc.) this should probably be

Suggested change
constexpr bool IsKernelArgumentTriviallyCopyable_v = IsKernelArgumentTriviallyCopyable<T>::value;
inline constexpr bool isKernelArgumentTriviallyCopyable = IsKernelArgumentTriviallyCopyable<T>::value;

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@fwyzard fwyzard left a comment

Choose a reason for hiding this comment

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

@psychocoderHPC I would follow the suggestion from @MaxSagebaum , and also change the variable template name to isKernelArgumentTriviallyCopyable<T>.

include/alpaka/kernel/Traits.hpp Outdated Show resolved Hide resolved
///
/// @tparam T type to check
/// @{
template<typename T>
Copy link
Member

Choose a reason for hiding this comment

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

You guys are going to love requires clauses in C++20.

};

template<typename T>
constexpr bool IsKernelArgumentTriviallyCopyable_v = IsKernelArgumentTriviallyCopyable<T>::value;
Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

@psychocoderHPC psychocoderHPC force-pushed the topic-kernelArgementsConditions branch 3 times, most recently from d859b60 to 1373c87 Compare December 1, 2023 16:35
bernhardmgruber
bernhardmgruber previously approved these changes Dec 1, 2023
// should be at least empty then (stateless).
std::is_empty_v<T> || std::is_trivially_copyable_v<T>,
"The kernel argument T must be trivially copyable!");
static_assert(IsKernelArgumentTriviallyCopyable_v<T>, "The kernel argument T must be trivially copyable!");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static_assert(IsKernelArgumentTriviallyCopyable_v<T>, "The kernel argument T must be trivially copyable!");
static_assert(isKernelArgumentTriviallyCopyable<T>, "The kernel argument T must be trivially copyable!");

Copy link
Member

Choose a reason for hiding this comment

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

Just one fix to get it compiling.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the usage to isKernelArgumentTriviallyCopyable, alpaka uses different naming compared to std.
e.g. isQueue,isPlatform, ...

fix alpaka-group#2195

Provide an alpaka specific trait to validate kernel argument conditions which can be specialized by the user
for their own types and at their own risk.
@bernhardmgruber bernhardmgruber merged commit 188361b into alpaka-group:develop Dec 4, 2023
23 checks passed
@psychocoderHPC psychocoderHPC deleted the topic-kernelArgementsConditions branch January 26, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

std::is_trivially_copyable too strong as a constraint
4 participants