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 MDRangePolicy #5516

Merged
merged 8 commits into from Mar 8, 2024
Merged

CTAD (deduction guides) for MDRangePolicy #5516

merged 8 commits into from Mar 8, 2024

Conversation

nliber
Copy link
Contributor

@nliber nliber commented Oct 3, 2022

Refactoring #5319 into smaller P/Rs

@nliber nliber mentioned this pull request Oct 3, 2022
Closed
@dalg24
Copy link
Member

dalg24 commented Oct 4, 2022

Virtually all the builds are failing on the ORNL Jenkins CI server (does not compile)
https://cloud.cees.ornl.gov/jenkins-ci/blue/organizations/jenkins/Kokkos/detail/Kokkos/10820/pipeline

@dalg24
Copy link
Member

dalg24 commented Oct 5, 2022

Retest this please

@nliber
Copy link
Contributor Author

nliber commented Oct 26, 2022

Tests are all compile-time now.

@nliber nliber added this to the Release 4.1 milestone May 8, 2023
@nliber nliber force-pushed the ctad-mdrangepolicy branch 2 times, most recently from 7335167 to 919c31d Compare July 8, 2023 01:04
masterleinad
masterleinad previously approved these changes Jul 10, 2023
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.

Looks OK to me.

@nliber nliber force-pushed the ctad-mdrangepolicy branch 2 times, most recently from 769b341 to a4558ff Compare July 11, 2023 18:14
@masterleinad masterleinad dismissed their stale review July 11, 2023 18:32

Changes after approval.

@nliber nliber mentioned this pull request Jul 31, 2023
Open
fnrizzi
fnrizzi reviewed Sep 11, 2023
View reviewed changes
core/src/KokkosExp_MDRangePolicy.hpp
struct MDRangePolicy;

// Note: If MDRangePolicy has a primary template, implicit CTAD (deduction
// guides) are generated -> MDRangePolicy<> by some compilers, which is
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, which compilers do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implicit guides are generated from the primary template.

Because there are no template parameters to deduce in a parameter pack, both clang and gcc deduce a matching ctor to MDRangePolicy<>, while nvcc will generate an error. Unfortunately, I find the standard wording on implicit guides to be fairly inscrutable (and cppreference doesn't help, as all it does is quote the standard) :-(. While I'd certainly like to know, practically speaking, it doesn't matter to us which one is correct, as we have to support all those compilers anyway.

Since MDRangePolicy always requires at least one template parameter (Rank), any guides that deduce to MDRangePolicy<> are just wrong.

nliber and others added 8 commits March 7, 2024 20:04
@nliber
CTAD for MDRangePolicy
1ec832a
@nliber
WIP: Moved CTAD tests from TestMDRangePolicyConstructors.hpp to
10c6364 
TestMDRangePolicyCTAD.cpp
@nliber
WIP Added [[maybe_unused]] to all static inline variables in
ce6d27b 
CTAD tests
@nliber
Removed TEST_EXECSPACE from the MDRangePolicy CTAD tests
4c5e80e 
(as that isn't needed for compile time only tests) and
associated cleanup
@nliber
Marked
6377e32 
struct ImplicitlyConvertibleToDefaultExecutionSpace::operator Kokkos::DefaultExecutionSpace() const;
as [[maybe_unused]] to make it consistent with the
other policy CTAD tests (in other PRs)
@nliber
Added a Workaround for nvc++ (CUDA-11.7-NVHPC) ignoring
24d1277 
[[maybe_unused]] 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"
b4c68c0
@nliber
Added MDRangePolicy CTAD tests for initializer_lists
eea5079 
Fixed a comment
masterleinad
masterleinad approved these changes Mar 8, 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.

Looks OK to me. Maybe fix the double whitespace on the comments.

core/src/KokkosExp_MDRangePolicy.hpp Show resolved Hide resolved
core/src/KokkosExp_MDRangePolicy.hpp Show resolved Hide resolved
@nliber
Copy link
Contributor Author

nliber commented Mar 8, 2024

Old typing habits last forever... :-)

@dalg24
Copy link
Member

dalg24 commented Mar 8, 2024

Ignoring the one HIP failure.

@dalg24 dalg24 merged commit cfc260a into kokkos:develop Mar 8, 2024
32 of 33 checks passed
@dalg24 dalg24 mentioned this pull request Mar 8, 2024
Closed
@ndellingwood ndellingwood mentioned this pull request Mar 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalg24 dalg24 dalg24 approved these changes

@fnrizzi fnrizzi fnrizzi left review comments

@masterleinad masterleinad masterleinad approved these changes

@crtrott crtrott crtrott approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 4.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nliber @dalg24 @masterleinad @crtrott @fnrizzi