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.

Sign up for GitHub

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

Jump to bottom

CTAD Deduction guides for TeamPolicy v2 #6117

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from
Open

CTAD Deduction guides for TeamPolicy v2 #6117

wants to merge 24 commits into from

Conversation

nliber
Copy link
Contributor

@nliber nliber commented May 10, 2023

Seeing if adding an Impl::TeamPolicyCommon between TeamPolicy and Impl::TeamPolicyInternal will get around the compiler ICEing bugs.

@nliber nliber mentioned this pull request May 10, 2023
Draft
@nliber nliber force-pushed the ctad-teampolicy-crtp branch 2 times, most recently from ddafb16 to 8c9f00a Compare June 23, 2023 18:07
@nliber
Copy link
Contributor Author

nliber commented Jun 26, 2023 •
edited

In order to get around various compiler bugs with regards to deduction guides, the following changes were made to TeamPolicy:

Introduction of Impl::TeamPolicyCommon, and move the implementation of TeamPolicy into it. This is done so as not to have duplicate code. The public inheritance hierarchy looks like

Before: TeamPolicy <- TeamPolicyInternal
After: TeamPolicy <- TeamPolicyCommon <- TeamPolicyInternal

TeamPolicyInternal could not be used for this. While it may look like there is a primary template for TeamPolicyInternal with common code, it turns out that it is specialized for every backend. The primary template definition for TeamPolicyInternal is not used for anything, other than being an archetype for anyone implementing a backend.

TeamPolicyCommon deliberately does not have a primary template, as this combined with a parameter pack was causing the deduction guides to ICE under certain compilers (nvcc). It only has a partial specialization.

In the partial specialization of TeamPolicyCommon, the first template parameter is the ExecutionSpace. This was picked because it is also the first template parameter needed for TeamPolicyInternal, although any parameter could have been used (including just ignoring it). The key part is that this implementation is a specialization of the undefined primary template.

Initially, when the constructors were moved from TeamPolicy into TeamPolicyCommon, the TeamPolicy constructors were replaced with inheriting constructors. This resulted in a compiler bug (icpc) due to inheriting constructors not working correctly when the inherited constructors had defaulted parameters, so the constructors in TeamPolicyCommon were changed so that none of them include defaulted parameters.

For some compilers, an inline static variable isn't considered as used when only in an unevaluated context (as they are in the compile time tests), so they are now marked as [[maybe_unused]].

@nliber
Copy link
Contributor Author

nliber commented Jul 3, 2023

retest this please

@nliber nliber force-pushed the ctad-teampolicy-crtp branch 2 times, most recently from 25e2e0d to 7ee8129 Compare July 11, 2023 17:51
@nliber nliber marked this pull request as ready for review July 12, 2023 17:54
masterleinad
masterleinad reviewed Jul 12, 2023
View reviewed changes
core/src/Kokkos_ExecPolicy.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_ExecPolicy.hpp Outdated Show resolved Hide resolved
@nliber nliber mentioned this pull request Jul 31, 2023
Open
@nliber nliber force-pushed the ctad-teampolicy-crtp branch 2 times, most recently from 7c9afe9 to e10abac Compare March 13, 2024 17:26
@nliber nliber changed the title Deduction guides for TeamPolicy v2 CTAD Deduction guides for TeamPolicy v2 Mar 13, 2024
nmm0
nmm0 reviewed Apr 1, 2024
View reviewed changes
Copy link
Contributor

@nmm0 nmm0 left a comment

Choose a reason for hiding this comment

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

Some initial comments

core/src/Kokkos_ExecPolicy.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_ExecPolicy.hpp Show resolved Hide resolved
core/src/Kokkos_ExecPolicy.hpp Show resolved Hide resolved
core/src/Kokkos_ExecPolicy.hpp Show resolved Hide resolved
nliber and others added 13 commits April 2, 2024 16:32
@nliber
CTAD for TeamPolicy
df17b39
@nliber
Replaced the primary template for TeamPolicy with
5a9e620 
a forward declaration requiring at least one (defaulted)
template parameter and two partial specializations TeamPolicy<P> and
TeamPolicy<P1, P2, Properties...>.
@nliber
Fixed comment formatting
40620f6
@nliber
Added compile time CTAD tests for TeamPolicy
bff1dd0
@nliber
Changed static variables to inline static variables in
329f443 
TestTeamPolicyConstructors to avoid internal linkage problems
with various compilers and compile time tests
@nliber
Moved the implmentation of TeamPolicy into Impl::TeamPolicyCommon
2f47532
@nliber
Fixed parameter pack bug
0efcc6e
@nliber
Fixed inheriting constructors
dbb1294 
Added some static_asserts enforcing TeamPolicyCommon usage assumptions
@nliber
Removed static_assert with incomplete type.
bd8a5ac 
Added some comments
@nliber
Fixed documentation
c417277
@nliber
Changed the constructors in TeamPolicyCommon to not use default
a7b4ada 
parameters to see if this fixes the issue with icpc and OPENMP builds
@nliber
Moved TeamPolicy CTAD tests into its own file and now build
cefc93d 
it under COMPILE_ONLY_SOURCES
@nliber
Added [[maybe_unused]] to the static inline variables
0123499 
in TestTeamPolicyCTAD, as they are never referenced
outside of unevaluated contexts.
nliber and others added 11 commits April 2, 2024 16:32
@nliber
Cleanup
6f75121
@nliber
Cleanup
76cd5dd
@nliber
Moved "execution_policy" typedef out of TeamPolicyCommon and
f6c63e7 
into TeamPolicy, as TeamPolicyCommon is not an execution policy.
Added the private typedef "team_policy" for use inside TeamPolicyCommon.
@nliber
CUDA-11.7-NVHPC warns
93590b2 
        ImplicitlyConvertibleToDefaultExecutionSpace::operator Kokkos::DefaultExecutionSpace" was declared but never referenced
because it is only referenced in unevaluated contexts.
Marking it as [[maybe_unused]]
@nliber
Added a Workaround for nvc++ (CUDA-11.7-NVHPC) ignoring [[maybe_unuse…
e766c45 
…d]] on

ImplicitlyConvertibleToDefaultExecutionSpace::operator
Kokkos::DefaultExecutionSpace() const

by defining it and implicitly calling it in another [[maybe_unused]] static inline
variable.
@nliber
Workaround for HIP-ROCm-5.2 "declared but never referenced"
e5b83d1
@nliber
Removed #if 0 block containing the original TeamPolicy implementation
32b44fd
@nliber
Removed superfluous friend declaration
f9ccee4
@nliber
Removed superfluous include
73a1372
@nliber
Simplified internal_policy
8df4d9f
@nliber
The comment block for TeamPolicyCommon is no longer
208c778 
doxygen syntax
masterleinad
masterleinad approved these changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

OK with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalg24 dalg24 dalg24 left review comments

@nmm0 nmm0 nmm0 left review comments

@masterleinad masterleinad masterleinad approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nliber @dalg24 @masterleinad @nmm0