Allow Kokkos Tools Interface to be built without Kokkos #4365

DavidPoliakoff · 2021-09-27T14:52:44Z

Closes #4327 (and #4326, for a bonus).

This is a refactoring of the Tools infrastructure to allow Kokkos Tools to be used without Kokkos. In this PR, all I do is change how Tools is organized such that this is possible. Given permission, I'll share my repo that tests this functionality. This would allow other programming models (RAJA, or Sameer's collaborators) to use Kokkos Tools functionality, allow codes to use only Kokkos Tools, and ideally clean up our code.

This is still a WIP, I need to test more paths through this code, but it currently achieves its goals of allowing independent builds, and (I believe, CI will determine this) doesn't interfere with Kokkos builds

…essible functionality

DavidPoliakoff · 2021-09-27T14:53:55Z

Update: hahaoops. Definitely interferes with Kokkos builds. But this is a first step

DavidPoliakoff · 2021-09-27T15:02:30Z

TODO: also have a Kokkos::Tools::parse_environment_variables edit: TODONE

…off/kokkos into feature/reorg-isolate-tuning

mhoemmen · 2021-09-27T21:46:31Z

Thanks David! : - D I know I'm not a paying customer at the moment, but I would prefer a design where the Kokkos Tools interface were factored out of KokkosCore proper, rather than #ifdef'd. That would make downstream applications that love to hard-code their build systems grumpy, though.

DavidPoliakoff · 2021-09-28T13:23:03Z

@mhoemmen : yeah, I'm right there with you. There's a model that achieves what you're asking for. Let me think about it. Actually, the ToolProgrammingInterface construct I added ages ago might be really helpful here, some of the stuff that's ifdef'd out is contained in there. Yeah, I think there's a path to what you want, I'll take a look today

DavidPoliakoff · 2021-09-28T17:46:34Z

Okay, I'm going to recommend we review this as is, just see if it is an acceptable solution. All the ways of avoiding ifdefs have some small problems. Once we have the orthogonalization in place (which I want anyway), we can look at some cleaner, but backwards compatible methods of achieving this

mhoemmen · 2021-09-28T19:28:54Z

I'm going to recommend we review this as is, just see if it is an acceptable solution.

I am cool with this -- thanks for considering my use cases! : - )

core/src/impl/Kokkos_Error.cpp

core/src/impl/Kokkos_Core.cpp

Rombur

This looks fine to me. I only have minor comments.

Rombur · 2021-11-03T14:30:57Z

core/src/Kokkos_Core.hpp

+  bool tune_internals;            // deprecated
+  bool tool_help        = false;  // deprecated
+  std::string tool_lib  = {};     // deprecated
+  std::string tool_args = {};     // deprecated


I agree with @masterleinad, I don't see how the variable are deprecated. Don't you just change the way they are initialized?

core/src/impl/Kokkos_Command_Line_Parsing.cpp

core/unit_test/tools/TestIndependence.cpp

core/src/Kokkos_Core.hpp

core/src/impl/Kokkos_Command_Line_Parsing.cpp

core/src/impl/Kokkos_Command_Line_Parsing.hpp

core/src/impl/Kokkos_Core.cpp

dalg24 · 2021-11-10T15:10:28Z

core/src/impl/Kokkos_Core.cpp

+  if (tools_init_arguments.tune_internals !=
+      Kokkos::Tools::InitArguments::PossiblyUnsetOption::unset) {
+    tune_internals = (tools_init_arguments.tune_internals ==
+                      Kokkos::Tools::InitArguments::PossiblyUnsetOption::on)
+                         ? true
+                         : false;


Why do you need a ternary operator here? Why is it not just

if (<not unset>) tune_internals = (tools_init_arguments.tune_internals == Kokkos::Tools::InitArguments::PossiblyUnsetOption::on);

Same comment below for the help

crtrott

left a few comments

core/src/impl/Kokkos_ViewMapping.hpp

crtrott · 2021-10-28T16:43:56Z

core/unit_test/tools/TestBuiltinTuners.cpp

@@ -57,11 +58,11 @@ int main(int argc, char* argv[]) {
    Kokkos::MDRangePolicy<Kokkos::Rank<2>> mdp({0, 0}, {1, 1});
    Kokkos::Tools::Experimental::TeamSizeTuner team_tune_this(
        "team_tuner", teamp, TestTeamFunctor{}, Kokkos::ParallelForTag{},
-        Kokkos::Tools::Impl::Impl::SimpleTeamSizeCalculator{});
+        Kokkos::Tools::Experimental::Impl::Impl::SimpleTeamSizeCalculator{});


Impl::Impl::?

core/src/impl/Kokkos_Command_Line_Parsing.hpp

dalg24 · 2021-11-10T15:16:33Z

core/src/impl/Kokkos_Core.cpp

+  auto initialization_status =
+      Kokkos::Tools::Impl::initialize_tools_subsystem(args);
+  if (initialization_status.result ==
+      Kokkos::Tools::Impl::InitializationStatus::InitializationResult::


Technically, you can directly write Kokkos::Tools::Impl::InitializationStatus::success. Did you mean for Kokkos::Tools::Impl::InitializationStatus::InitializationResult to be a strong enumeration?

No strong feelings about this one, game to spell it however

core/src/impl/Kokkos_Profiling.cpp

core/src/impl/Kokkos_Profiling.hpp

core/src/impl/Kokkos_Command_Line_Parsing.cpp

Co-authored-by: Damien L-G <dalg24+github@gmail.com>

DavidPoliakoff · 2021-11-10T18:11:15Z

Somehow still failing format :/

DavidPoliakoff · 2021-11-10T18:17:24Z

Added a newline at the end of the file, reran clang-format, now it's passing

nmhamster · 2021-11-10T23:55:52Z

@crtrott - is there any possibility we can merge this before @DavidPoliakoff leaves Sandia?

nmhamster · 2021-11-11T16:51:45Z

@crtrott / @dalg24 - thanks for the help with this one.

DavidPoliakoff added 8 commits August 26, 2021 10:38

Separated generic begin_parallel_x events and tuning from backend-acc…

5218e17

…essible functionality

Intermediate commit: have builds without use of Kokkos

680f3c1

Intermediate commit: parsing (midway)

fc3f6b4

Intermediate commit

ace36b6

First swing at orthogonalizing tools core from Kokkos core

871f1b9

Format

7df427c

Fixes for Kokkos Tools

d29c8d3

Format

9d3b5dc

DavidPoliakoff added 10 commits September 27, 2021 09:43

Updates with initialize(InitArguments)

bae94b9

Merge branch 'feature/reorg-isolate-tuning' of github.com:DavidPoliak…

4a7287f

…off/kokkos into feature/reorg-isolate-tuning

Format

dc463c5

Commit with tune_internals fix

eb2287e

Format

d7bfa43

Added tune_internals back

e2a1cc8

Format

7627ef1

Fixed inconsistencies

f389458

Merge branch 'feature/reorg-isolate-tuning' of github.com:DavidPoliak…

abe1569

…off/kokkos into feature/reorg-isolate-tuning

Format

f426d2f

DavidPoliakoff marked this pull request as ready for review September 27, 2021 18:45

DavidPoliakoff added 2 commits September 27, 2021 13:37

Made tool args consistent

b390d30

Format

fd5bc3c

dalg24 reviewed Sep 29, 2021

View reviewed changes

core/src/impl/Kokkos_Error.cpp Outdated Show resolved Hide resolved

core/src/impl/Kokkos_Core.cpp Outdated Show resolved Hide resolved

DavidPoliakoff added 2 commits September 29, 2021 13:03

Removed forward declarations

d1aee4d

Format

b7f2605

Rombur reviewed Nov 3, 2021

View reviewed changes

DavidPoliakoff added 2 commits November 3, 2021 11:10

Addressed Bruno's comments (thanks!)

af698ca

Format

386b6f3

Rombur approved these changes Nov 3, 2021

View reviewed changes

dalg24 reviewed Nov 10, 2021

View reviewed changes

crtrott requested changes Nov 10, 2021

View reviewed changes

Developer: David P automation moved this from Awaiting Feedback to In progress Nov 10, 2021

dalg24 reviewed Nov 10, 2021

View reviewed changes

core/src/impl/Kokkos_Profiling.cpp Show resolved Hide resolved

DavidPoliakoff added 2 commits November 10, 2021 08:36

Address review comments

9f33083

Format

9f5c236

dalg24 reviewed Nov 10, 2021

View reviewed changes

core/src/impl/Kokkos_Profiling.hpp Outdated Show resolved Hide resolved

DavidPoliakoff added 2 commits November 10, 2021 09:39

More review comments

132509d

Format

2867b86

dalg24 reviewed Nov 10, 2021

View reviewed changes

core/src/impl/Kokkos_Command_Line_Parsing.cpp Show resolved Hide resolved

Update core/src/impl/Kokkos_Command_Line_Parsing.cpp

d913f9f

Co-authored-by: Damien L-G <dalg24+github@gmail.com>

DavidPoliakoff added 2 commits November 10, 2021 11:11

Rereformat

87cc6a5

Rerereformat

87c1d46

DavidPoliakoff moved this from In progress to Awaiting Feedback in Developer: David P Nov 10, 2021

dalg24 approved these changes Nov 11, 2021

View reviewed changes

crtrott approved these changes Nov 11, 2021

View reviewed changes

crtrott merged commit ff083d4 into kokkos:develop Nov 11, 2021

Kokkos Release 3.6 automation moved this from Awaiting Feedback to Done Nov 11, 2021

DavidPoliakoff moved this from Awaiting Feedback to Done in Developer: David P Nov 11, 2021

DavidPoliakoff mentioned this pull request Nov 11, 2021

Kokkos Tools first attempt LLNL/RAJA#1158

Open

4 tasks

ndellingwood mentioned this pull request Nov 12, 2021

Nightly Hip builds unit tests seg fault #4527

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Kokkos Tools Interface to be built without Kokkos #4365

Allow Kokkos Tools Interface to be built without Kokkos #4365

DavidPoliakoff commented Sep 27, 2021

DavidPoliakoff commented Sep 27, 2021

DavidPoliakoff commented Sep 27, 2021 •

edited

mhoemmen commented Sep 27, 2021

DavidPoliakoff commented Sep 28, 2021

DavidPoliakoff commented Sep 28, 2021

mhoemmen commented Sep 28, 2021

Rombur left a comment

Rombur Nov 3, 2021

dalg24 Nov 10, 2021

crtrott left a comment

crtrott Oct 28, 2021

dalg24 Nov 10, 2021

DavidPoliakoff Nov 10, 2021

DavidPoliakoff commented Nov 10, 2021

DavidPoliakoff commented Nov 10, 2021

nmhamster commented Nov 10, 2021

nmhamster commented Nov 11, 2021

Allow Kokkos Tools Interface to be built without Kokkos #4365

Allow Kokkos Tools Interface to be built without Kokkos #4365

Conversation

DavidPoliakoff commented Sep 27, 2021

DavidPoliakoff commented Sep 27, 2021

DavidPoliakoff commented Sep 27, 2021 • edited

mhoemmen commented Sep 27, 2021

DavidPoliakoff commented Sep 28, 2021

DavidPoliakoff commented Sep 28, 2021

mhoemmen commented Sep 28, 2021

Rombur left a comment

Choose a reason for hiding this comment

Rombur Nov 3, 2021

Choose a reason for hiding this comment

dalg24 Nov 10, 2021

Choose a reason for hiding this comment

crtrott left a comment

Choose a reason for hiding this comment

crtrott Oct 28, 2021

Choose a reason for hiding this comment

dalg24 Nov 10, 2021

Choose a reason for hiding this comment

DavidPoliakoff Nov 10, 2021

Choose a reason for hiding this comment

DavidPoliakoff commented Nov 10, 2021

DavidPoliakoff commented Nov 10, 2021

nmhamster commented Nov 10, 2021

nmhamster commented Nov 11, 2021

DavidPoliakoff commented Sep 27, 2021 •

edited