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

workDivMembers construction with a user defined type #2243

Merged

Conversation

mehmetyusufoglu
Copy link
Contributor

@mehmetyusufoglu mehmetyusufoglu commented Feb 26, 2024

WorkDivMembers class constructor could accept any vector type if the necessary traits and functions related to that type are provided.

This PR uses a user-defined vector, related traits and specializations to test the of WorkDivMembers constructors.

  • Tests are added to test the usage of the user-defined vectors in WorkDivMembers construction.
  • Already existing WorkDivMembers construction tests are improved and refactored.
  • Initializer list test is added in construction of WorkDivMembers. auto const workDiv2DUsingInitList = alpaka::WorkDivMembers<Dim2D, Idx>({6, 9}, {4, 6}, {1, 1}); initializes 3 alpaka vectors as arguments.

@mehmetyusufoglu mehmetyusufoglu changed the title Simplify WorkDiv Creation: Use std::array arguments and no explicit class parameters [onto PR 2240] Simplify WorkDiv Creation: accept std::array args, don't require explicit class params Feb 26, 2024
@mehmetyusufoglu mehmetyusufoglu changed the title [onto PR 2240] Simplify WorkDiv Creation: accept std::array args, don't require explicit class params [after PR 2240] Simplify WorkDiv: accept std::array args, don't require explicit class template types Feb 26, 2024
@mehmetyusufoglu mehmetyusufoglu changed the title [after PR 2240] Simplify WorkDiv: accept std::array args, don't require explicit class template types [after merging PR 2240] Simplify WorkDiv: accept std::array args, don't require explicit class template types Feb 26, 2024
@mehmetyusufoglu mehmetyusufoglu changed the title [after merging PR 2240] Simplify WorkDiv: accept std::array args, don't require explicit class template types [after merging PR 2240] simplify workdiv: accept std::array, not require class template param Feb 26, 2024
@fwyzard
Copy link
Contributor

fwyzard commented Feb 26, 2024

Rather than std::arrays (or in addition to them, if there is any concrete use case), could you add support for [std::initializer_list](https://en.cppreference.com/w/cpp/utility/initializer_list) arguments ?

That would support alpaka::WorkDivMembers{{1u, 1u}, {2u, 2u}, {1u, 1u}};.

//! \tparam TVal is the element type of the array
//! \tparam N is the size of the array
template<typename TVal, size_t N>
ALPAKA_FN_HOST_ACC constexpr auto arrayToVec(std::array<TVal, N> const& a) -> Vec<DimInt<N>, TVal>
Copy link
Member

Choose a reason for hiding this comment

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

There is no need for an additional method that can only handle std array.
We have the traits GetExtents to provide the translation that alpaka can work with alpaka foreign types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I used getExtents. There is already one implemented for 1D array arguments in "ViewPlanePtr" therefore I had to check the dimension. I will refine the solution. Thanks a lot.

@mehmetyusufoglu mehmetyusufoglu changed the title [after merging PR 2240] simplify workdiv: accept std::array, not require class template param simplify workdiv: accept std::array, not require class template param Feb 26, 2024
@bernhardmgruber
Copy link
Member

That would support alpaka::WorkDivMembers{{1u, 1u}, {2u, 2u}, {1u, 1u}};.

This may already work with #2240, if the right constructor is selected. I think alpaka::Vec can be created from an initializer list.

@bernhardmgruber
Copy link
Member

That would support alpaka::WorkDivMembers{{1u, 1u}, {2u, 2u}, {1u, 1u}};.

This may already work with #2240, if the right constructor is selected. I think alpaka::Vec can be created from an initializer list.

It turns out it doesn't work, and I am not exactly sure why. The compiler fails to deduce the template parameters of the deduction guide. It has something to do with {1u, 1u} being an initializer list, because alpaka::WorkDivMembers{alpaka::Vec{1u, 1u}, {2u, 2u}, {1u, 1u}}; works successfully.

@mehmetyusufoglu
Copy link
Contributor Author

mehmetyusufoglu commented Feb 26, 2024 via email

@mehmetyusufoglu mehmetyusufoglu marked this pull request as draft February 27, 2024 07:46
@mehmetyusufoglu mehmetyusufoglu marked this pull request as ready for review February 27, 2024 09:07
@mehmetyusufoglu mehmetyusufoglu marked this pull request as draft February 27, 2024 09:34
@mehmetyusufoglu mehmetyusufoglu force-pushed the WorkDivWithArrayParam branch 2 times, most recently from d27f8c2 to fc184b8 Compare March 1, 2024 16:07
@mehmetyusufoglu mehmetyusufoglu force-pushed the WorkDivWithArrayParam branch 6 times, most recently from 9ca42be to c6594df Compare March 4, 2024 17:54
@mehmetyusufoglu mehmetyusufoglu changed the title simplify workdiv: accept std::array, not require class template param Add a user defined vector and specializations to test workDivMembers construction with arbitrary type Mar 4, 2024
@mehmetyusufoglu mehmetyusufoglu changed the title Add a user defined vector and specializations to test workDivMembers construction with arbitrary type workDivMembers construction with a user defined type Mar 5, 2024
@mehmetyusufoglu mehmetyusufoglu marked this pull request as ready for review March 5, 2024 03:37
@mehmetyusufoglu mehmetyusufoglu force-pushed the WorkDivWithArrayParam branch 2 times, most recently from 9fb26bf to 98f6601 Compare March 5, 2024 04:30
include/alpaka/extent/Traits.hpp Outdated Show resolved Hide resolved
test/unit/workDiv/CMakeLists.txt Outdated Show resolved Hide resolved
test/unit/workDiv/CMakeLists.txt Show resolved Hide resolved
test/unit/workDiv/src/WorkDivHelpersTest.cpp Outdated Show resolved Hide resolved
@mehmetyusufoglu
Copy link
Contributor Author

Rather than std::arrays (or in addition to them, if there is any concrete use case), could you add support for [std::initializer_list](https://en.cppreference.com/w/cpp/utility/initializer_list) arguments ?

That would support alpaka::WorkDivMembers{{1u, 1u}, {2u, 2u}, {1u, 1u}};.

Currently the following are possible due to a merged PR: alpaka::WorkDivMembers<Dim2D,Idx>{{1u, 1u}, {2u, 2u}, {1u, 1u}}; . OR alpaka::WorkDivMembers{Vec{1u, 1u}, Vec{2u, 2u}, Vec{1u, 1u}}; Vec is alpaka::vec with specific Dim and Idx.

I will consider your suggestion together with C++20, seems more reasonable I think.

Copy link
Member

@bernhardmgruber bernhardmgruber left a comment

Choose a reason for hiding this comment

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

Here are a few comments:

Comment on lines 92 to 105
template<typename T, typename = void>
struct has_member_arr : std::false_type
{
};

template<typename T>
struct has_member_arr<T, std::void_t<decltype(&T::arr)>> : std::true_type
{
};

//! The DimType specialization for the user defined vector which has a member called "arr"
//! \tparam T The vector type
template<typename T>
struct DimType<T, std::enable_if_t<has_member_arr<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.

Suggested change
template<typename T, typename = void>
struct has_member_arr : std::false_type
{
};
template<typename T>
struct has_member_arr<T, std::void_t<decltype(&T::arr)>> : std::true_type
{
};
//! The DimType specialization for the user defined vector which has a member called "arr"
//! \tparam T The vector type
template<typename T>
struct DimType<T, std::enable_if_t<has_member_arr<T>::value>>
//! The DimType specialization for the user defined vector which has a member called "arr"
//! \tparam T The vector type
template<typename T>
struct DimType<FooVec>

Why don't you just specialize for FooVec directly? I don't see a need for SFINAE here.

Applies to other traits as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FooVec is a template, not a type. But i will try to formulate your approach.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, my bad. Should be:

template<typename TVec, std::size_t Size>
struct DimType<FooVec<TVec, Size>>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done thanks.

test/unit/workDiv/src/FooVec.hpp Outdated Show resolved Hide resolved
@mehmetyusufoglu
Copy link
Contributor Author

mehmetyusufoglu commented Mar 8, 2024 via email

@psychocoderHPC psychocoderHPC merged commit d9348ea into alpaka-group:develop Mar 14, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants