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

simplification of workdiv creation #2240

Merged
merged 1 commit into from Feb 27, 2024

Conversation

mehmetyusufoglu
Copy link
Contributor

@mehmetyusufoglu mehmetyusufoglu commented Feb 19, 2024

Workdiv constructor needs 2 class template parameters and 3 alpaka::Vec arguments.
auto const workdiv = alpaka::WorkDivMembers<Dim, Idx>{arraySize / blockSize, (int)blockSize, 1u};

The aim is being able to create workdiv, without template parameters, for example
auto const workdiv = createWorkDiv(arraySize / blockSize, blockSize, 1);

Update: Factory method is not used, a new constructor is introduced, no need for explicit type params in the new constructor call. All 3 input types must be alpaka::Vec with the same TDim type. auto const workDiv2D = alpaka::WorkDivMembers(blocksPerGrid2D, threadsPerBlock2D, elementsPerThread2D);

Another PR [2243] is provided onto this one, for using std::array instead of alpaka vectors as arguments. Class template parameters are not needed.

@mehmetyusufoglu mehmetyusufoglu marked this pull request as draft February 19, 2024 12:46
SimeonEhrig
SimeonEhrig previously approved these changes Feb 20, 2024
@mehmetyusufoglu mehmetyusufoglu changed the title [WIP] Factory method for simplification of workdiv creation Factory method for simplification of workdiv creation Feb 20, 2024
@mehmetyusufoglu mehmetyusufoglu marked this pull request as ready for review February 20, 2024 08:42
Comment on lines 385 to 396
//! \tparam TDim The dimensionality of the accelerator device properties.
//! \tparam TIdx The idx type of the accelerator device properties.
//! \return Returns the work division; namely the WorkDivMembers instance.
template<typename TIdx, typename TDim>
auto createWorkDiv(
alpaka::Vec<TDim, TIdx> const& gridBlockExtent,
alpaka::Vec<TDim, TIdx> const& blockThreadExtent,
alpaka::Vec<TDim, TIdx> const& elemExtent) -> alpaka::WorkDivMembers<TDim, TIdx>
{
return alpaka::WorkDivMembers<TDim, TIdx>(gridBlockExtent, blockThreadExtent, elemExtent);
}

Copy link
Member

Choose a reason for hiding this comment

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

Why can't we just add such a constructor to WorkDivMembers and a deduction guide if necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I wanted to use "create..." since I will add new similar functions. Using create functions seemed cleaner for conversions from stl container types and initializer lists.

By the way; I think you mean deduction guides for constructors as the one below, since they don't have a return type?:

template<class A, class B>
struct Agg
{
    A a;
    B b;
};
// implicitly-generated guides are formed from default, copy, and move constructors
 
template<class A, class B>
Agg(A a, B b) -> Agg<A, B>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently we cannot use stl containers, argument types are constrained to alpaka vector and buffer.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but I wanted to use "create..." since I will add new similar functions. Using create functions seemed cleaner for conversions from stl container types and initializer lists.

I beg to disagree. Constructors are the C++ way to construct an object. There are limited use cases for factory functions, but if a constructor will do fine, use one. You can overload them if you need to convert from various sources.

By the way; I think you mean deduction guides for constructors as the one below, since they don't have a return type?:

template<class A, class B>
struct Agg
{
    A a;
    B b;
};
// implicitly-generated guides are formed from default, copy, and move constructors
 
template<class A, class B>
Agg(A a, B b) -> Agg<A, B>;

Yes, I mean this kind of deduction guide. But it turns out, that one is not necessary here.

Currently we cannot use stl containers, argument types are constrained to alpaka vector and buffer.

Alright, you don't need one here :)

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 moved the createWorkDiv function as a new constructor. This PR could be merged, then I will prepare a second PR for the array or initilizer list type of inputs without explicit type parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needed a deduction guide (hence a seperate constructor declaration with deduction guide) for some compilers.

@bernhardmgruber
Copy link
Member

bernhardmgruber commented Feb 20, 2024

Just tried it, if you add the constructor:

        ALPAKA_FN_HOST_ACC WorkDivMembers(
            alpaka::Vec<TDim, TIdx> const& gridBlockExtent,
            alpaka::Vec<TDim, TIdx> const& blockThreadExtent,
            alpaka::Vec<TDim, TIdx> const& elemExtent)
            : m_gridBlockExtent(gridBlockExtent)
            , m_blockThreadExtent(blockThreadExtent)
            , m_threadElemExtent(elemExtent)
        {
        }

your unit test can be written using:

    auto ref = alpaka::WorkDivMembers(blocksPerGrid3D, threadsPerBlock3D, elementsPerThread3D);

and compiles and passes the tests.

include/alpaka/workdiv/WorkDivMembers.hpp Outdated Show resolved Hide resolved
include/alpaka/workdiv/WorkDivMembers.hpp Outdated Show resolved Hide resolved
Comment on lines 100 to 106
//! Deduction guide for the constructor which can be called without explicit template type parameters
ALPAKA_NO_HOST_ACC_WARNING
template<typename TDim, typename TIdx>
ALPAKA_FN_HOST_ACC explicit WorkDivMembers(
alpaka::Vec<TDim, TIdx> const& gridBlockExtent,
alpaka::Vec<TDim, TIdx> const& blockThreadExtent,
alpaka::Vec<TDim, TIdx> const& elemExtent) -> WorkDivMembers<TDim, TIdx>;

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
//! Deduction guide for the constructor which can be called without explicit template type parameters
ALPAKA_NO_HOST_ACC_WARNING
template<typename TDim, typename TIdx>
ALPAKA_FN_HOST_ACC explicit WorkDivMembers(
alpaka::Vec<TDim, TIdx> const& gridBlockExtent,
alpaka::Vec<TDim, TIdx> const& blockThreadExtent,
alpaka::Vec<TDim, TIdx> const& elemExtent) -> WorkDivMembers<TDim, TIdx>;
template<typename TDim, typename TIdx>
ALPAKA_FN_HOST_ACC WorkDivMembers(
alpaka::Vec<TDim, TIdx> const& gridBlockExtent,
alpaka::Vec<TDim, TIdx> const& blockThreadExtent,
alpaka::Vec<TDim, TIdx> const& elemExtent) -> WorkDivMembers<TDim, TIdx>;

I am not even sure if doxygen supports documenting deduction guides. I would just leave it here.

Different question: I did not need a deduction guide for g++-13. Is the deduction guide provided as a workaround for some compilers? If yes, could you maybe document them here, so we can remove the deduction guide when we phase out those compilers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were many compilers, yes I can document. Thanks. ( I might return to create... functions idea because now I am trying to call WorkDivMembers constructor without constraining the arguments to alpaka vector. )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment is important for the developers

@mehmetyusufoglu mehmetyusufoglu changed the title Factory method for simplification of workdiv creation Factory method for simplification of workdiv: Remove explicit class template need Feb 26, 2024
psychocoderHPC
psychocoderHPC previously approved these changes Feb 26, 2024
@psychocoderHPC psychocoderHPC added this to the 1.2.0 milestone Feb 26, 2024
@psychocoderHPC psychocoderHPC changed the title Factory method for simplification of workdiv: Remove explicit class template need simplification of workdiv creation Feb 26, 2024
@psychocoderHPC
Copy link
Member

we need to fix the OSX CI before we can merge this.

@psychocoderHPC
Copy link
Member

@mehmetyusufoglu could you please rebase this against the develop branch? I merged the OSX CI fix

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.

One more small nit:

include/alpaka/workdiv/WorkDivMembers.hpp Outdated Show resolved Hide resolved
@bernhardmgruber bernhardmgruber merged commit e01c0d7 into alpaka-group:develop Feb 27, 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