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

Allow extension of built-in tuners with additional tuning axes #3961

Merged
merged 14 commits into from
Apr 27, 2021

Conversation

DavidPoliakoff
Copy link
Contributor

Implements part 1 of #3960 . Right now, a user can't say "I want to tune what a team size does and my own thing" without making nested tuning problems, which get tricky very quickly. This allows that functionality, a user can simply call "combine" on an instance of one of our tuners, pass in a vector of items, and get back a tuner that tunes their axis as well as what the original tuner tuned. I also add tests for this. The intended use case is in Kokkos Kernels, in which to tune their SPMV, they need to tune the team size and vector length (TeamSizeTuner), but also need to tune the number of rows of a matrix assigned to each thread. Since the number of rows is orthogonal to the TeamPolicy configuration, they can simply combine that axis into a TeamSizeTuner and be good to go

Note that as of now the tests refer to things in namespace Impl. I'm considering moving those things out of the Impl namespace, but don't want to bloat this PR.

@masterleinad
Copy link
Contributor

Any chance that you get rid of the massive indentation changes in core/unit_test/CMakeLists.txt?

@DavidPoliakoff
Copy link
Contributor Author

I have no idea what set of clang-formats, IDE's, and other lovely scripts caused that. I'll try to get to it Monday

core/src/Kokkos_Tuners.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_Tuners.hpp Outdated Show resolved Hide resolved
core/src/Kokkos_Tuners.hpp Outdated Show resolved Hide resolved
Comment on lines +348 to 349
MultidimensionalSparseTuningProblem(StoredProblemSpace space,
const std::vector<std::string>& names)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since StoredProblemSpace is private, I guess this constructor can also be private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This constructor needs to be public. We construct tuners using this directly when we extend them

core/src/Kokkos_Tuners.hpp Show resolved Hide resolved
core/src/Kokkos_Tuners.hpp Outdated Show resolved Hide resolved
@@ -65,19 +65,29 @@ namespace Tools {
namespace Experimental {

// forward declarations
SetOrRange make_candidate_set(size_t size, int64_t* data);
SetOrRange make_candidate_set(size_t size, int64_t *data);

bool have_tuning_tool();
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all these whitespace changes intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I'm applying apply-clang-format. I don't want to worry about this kind of thing without a good reason.

core/src/Kokkos_Tuners.hpp Outdated Show resolved Hide resolved
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.

There are still a lot of whitespace changes to CMake files.

core/unit_test/CMakeLists.txt Outdated Show resolved Hide resolved
@masterleinad masterleinad dismissed their stale review April 26, 2021 22:52

CMakeLists.txt isn't changed anymore.

@masterleinad
Copy link
Contributor

The indentation is still off (due to my unindented suggestions). 🙂
Also, I would still prefer to keep the whitespace changes minimal. I can push a corresponding commit if you like.

@DavidPoliakoff
Copy link
Contributor Author

@masterleinad : feel free, though I think it's an iffy use of a person's time

@crtrott crtrott merged commit 11f2aa9 into kokkos:develop Apr 27, 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