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

Clean up AnalyzePolicy #3564

Merged
merged 2 commits into from
Mar 16, 2021
Merged

Conversation

dhollman
Copy link

@dhollman dhollman commented Nov 3, 2020

Right now, the size of the code for AnalyzePolicy scales quadraticly with the number of traits we analyze. With this change, the scaling is linear and adding a new trait doesn't have to touch any other code. There's nothing fancy going on here; we're just using partial specialization to detect the various traits. The one trick that was missing before is that we have to extract the work tag using an unspecialized template that defers to a base with a different name (which is partially specialized) before going back to the analysis of the rest of the traits.

@dalg24
Copy link
Member

dalg24 commented Nov 3, 2020

Retest this please

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.

Otherwise, this looks good AFAICT.

template <class... Traits>
struct AnalyzePolicy<void, Impl::IsGraphKernelTag, Traits...>
: AnalyzePolicy<void, Traits...> {
using is_graph_kernel = std::true_type;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are multiple GraphKernelTags allowed? Don't you need to make sure that none of the base class template parameters is Impl::IsGraphKernelTag?

Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

Fine by me

typename PolicyBase::launch_bounds,
typename PolicyBase::work_item_property, std::true_type>;
//------------------------------------------------------------------------------
// Work item propoerty case
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
// Work item propoerty case
// Work item property case

Comment on lines +187 to +291
// A tag class for dependent defaults that must be handled by the
// PolicyTraitsWithDefaults wrapper, since their defaults depend on other traits
struct dependent_policy_trait_default;
Copy link
Member

Choose a reason for hiding this comment

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

Does not need a definition?

@masterleinad
Copy link
Contributor

Apparently MSVC has some problems with this pull request. I tried to generate somewhat better error messages in https://github.com/masterleinad/kokkos/tree/analyze-policy-cleanup (with error messages in https://ci.appveyor.com/project/masterleinad/kokkos-5apfq/builds/36136850). One of them is saying

[00:07:46] C:\projects\source\core\src\impl/Kokkos_AnalyzePolicy.hpp(172,37): error C2338: Kokkos Error: More than one work tag given [C:\projects\source\build\core\unit_test\KokkosCore_UnitTest_Serial1.vcxproj]
[00:07:46] C:\projects\source\core\src\impl/Kokkos_AnalyzePolicy.hpp(189): message : see reference to class template instantiation 'Kokkos::Impl::TAssertEquality<void,_Ty>' being compiled [C:\projects\source\build\core\unit_test\KokkosCore_UnitTest_Serial1.vcxproj]
[00:07:46] with
[00:07:46] [
[00:07:46] _Ty=Kokkos::Impl::AnalyzePolicy<void,>::launch_bounds
[00:07:46] ]

so apparently launch_bounds becomes a work tag at some point (for MSVC).

@dalg24 dalg24 added this to the Tentative 3.4 Release milestone Nov 4, 2020
D. S. Hollman added 2 commits March 3, 2021 10:16
Right now, the size of the code for AnalyzePolicy scales quadratically
with the number of traits we analyze. With this change, the scaling is
linear and adding a new trait doesn't have to touch any other code.
The details of the runtime storage that may be associated with
a policy trait is now implemented only as part of the
trait-specific specializations. Traits are now fully disjoint with
the exception of defaults in AnalyzePolicy<void> and
PolicyTraitsWithDefaults
@masterleinad
Copy link
Contributor

/var/jenkins/workspace/Kokkos/core/src/impl/Kokkos_AnalyzePolicy.hpp: In member function 'Kokkos::Impl::AnalyzePolicy<void>& Kokkos::Impl::AnalyzePolicy<void>::operator=(const Kokkos::Impl::PolicyTraitsWithDefaults<Other>&)':
/var/jenkins/workspace/Kokkos/core/src/impl/Kokkos_AnalyzePolicy.hpp:329:56: error: no return statement in function returning non-void [-Werror=return-type]
   AnalyzePolicy& operator=(PolicyTraitsWithDefaults<Other> const&) {}
                                                        ^

dalg24 added a commit that referenced this pull request Mar 16, 2021
More extensive disentangling of Policy Traits (Replaces #3564)
@dalg24 dalg24 merged commit 200440f into kokkos:develop Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants