merge kp-kernels-timers

[blegouix]

Environment flag "KOKKOS_TOOLS_TIMER_JSON" determines if export is done in .dat or .json format.

Regions are also exported in the .json (it was not the case previously)

[masterleinad]

Can you please describe the behavior when setting KP_KERNEL_TIMER_JSON and when you are not setting it? Is something exported? What is printed? How did the behavior change in both cases?

[blegouix]

KP_KERNEL_TIMER_JSON=1, True or true leads to the behavior of previous kp_kernel_timer_json.cpp file (export of a .json file giving the timings), KP_KERNEL_TIMER_JSON with any other value or unset gives the behavior of previous kp_kernel_timer.cpp (export of a .dat file, readable with kp_reader)

Moreover, regions timings was not implemented in kp_kernel_timer_json.cpp. Now they are.

[vlkale]

Thanks @blegouix

I would like to request changing the environment variable name to 'KOKKOS_TOOLS_TIMER_JSON so as to be uniform with the other Kokkos Tools environment variables.

The format for tools environment variables is KOKKOS_TOOLS_<nameofTool>_<Variable>. It looks like this is not clear in the documentation of Kokkos Tools, and I will update this from my end.

[vlkale]

@blegouix

I just want to point it out that there seems to be an error in one of the CI build tests. It comes from PAPI connector. The error is in the screenshot below. The CI simple-build (without Kokkos) passes.

[blegouix]

I think the PAPI thing is just a warning, but there is indeed an error there:

/usr/bin/ld: ../profiling/all/libkokkostools.so: undefined reference toKokkosTools::KernelTimerJSON::get_event_set()'`

I'll check it, it is possible that namespace changed or something

[vlkale]

I think the PAPI thing is just a warning, but there is indeed an error there:

/usr/bin/ld: ../profiling/all/libkokkostools.so: undefined reference toKokkosTools::KernelTimerJSON::get_event_set()'`

I'll check it, it is possible that namespace changed or something

Great, thanks!

Yeah, the Kokkos Tools CI tests don’t pass unless all warnings are eliminated.

[blegouix]

Can you run the CI again ?

[blegouix]

Can you run the CI again ? Also, I do not think I have the right version of clang-format, could you format it for me ?

Thx,

[vlkale]

Can you run the CI again ?

Done.

Also, I do not think I have the right version of clang-format, could you format it for me ?

Thx,

Use clang-format-8.

On a Mac (if you have it), you can use homebrew and do brew install clang-format@8. Otherwise, can you download it from there on clang.llvm.org? I think it will help you to get clang-format-8 for PR contribution to Kokkos Tools. If you can't get this to work easily, I can do this for you.

[blegouix]

I have no idea why the remaining test fails :(

[blegouix]

Hello, can we merge this or do you think there are remaining things to do ?

[vlkale]

Started CI workflow.

I am OK with not sorting the times. I see a broader motivation though - this is how CUDA nvtx tools display timing output, and so there is an expectation by a Kokkos user for this may be similar.

I think the environment variable mentioned in your last message is a good temporary solution.

Thanks a lot for your help and work here!

[blegouix]

The environment variable is not in the scope of this MR right ?

Im on holidays next week so would be great if we can merge before that