Kernel logger upgrades for better output and better code comparisons by csiefer2 · Pull Request #276 · kokkos/kokkos-tools

csiefer2 · 2024-10-16T14:50:09Z

This PR does two things:

Upgrades the output on the Kernel logger to look more like the Tpetra DeepCopyTools --- rather than output space ID's we demanagle that and output names.
Allow you to use the KOKKOS_TOOLS_PROFILE_SUPPRESS_COUNTS variable to suppress the kernel counts in the output --- this is really useful if you're comparing two versions of the code with roughly the same flow, but some different kernel calls here and there. Specifically, code paths with and without UnifiedMemory. The call counting makes diff really hard and so turning it off can make diff easier.

csiefer2 · 2024-10-16T15:05:52Z

Yeah, yeah, I'll fix clang later. Right now I want feedback.

vlkale · 2024-10-16T18:52:04Z

The CI check formatting-check with clang-tidy is fine to not pass for now for the sake of a PR review. However, the CI check for simple-build is showing error: ‘int_for_synchronization_reason’ was not declared in this scope. It looks like the rest of the CI checks are also not passing, for the same reason.

Also, JM2C, but it may be good to separate this out into two PRs. I think the first change does not depend on whether the Kokkos Tools user sets KOKKOS_PROFILE_SUPPRESS_COUNTS, AFAIS. I also think the first change isn't controversial while the second change will need more time / thinking IMO: I wonder if the second change has a more efficient implementation given the conditionals in it. This is just a suggestion from my experience - feel free to keep it as is if you think it's better as one PR.

csiefer2 · 2024-10-16T21:49:30Z

@vlkale This is a debugging tool that fences all over the place. Host-side conditionals are basically irrelevant in terms of the performance hit the fences create.

As for the compile error, that's because the headers are no longer consistent with the headers in Kokkos. I'll add a fix to that in a second.

Signed-off-by: Chris Siefert <csiefer@sandia.gov>

csiefer2 · 2024-10-16T22:02:27Z

Compiles just fine now...

Signed-off-by: Chris Siefert <csiefer@sandia.gov>

masterleinad · 2024-10-17T14:08:19Z

+  if (eid.type == DeviceType::Serial)
+    device_label += "Serial";
+  else if (eid.type == DeviceType::OpenMP)
+    device_label += "OpenMP";
+  else if (eid.type == DeviceType::Cuda)
+    device_label += "Cuda";
+  else if (eid.type == DeviceType::HIP)
+    device_label += "HIP";
+  else if (eid.type == DeviceType::OpenMPTarget)
+    device_label += "OpenMPTarget";
+  else if (eid.type == DeviceType::HPX)
+    device_label += "HPX";
+  else if (eid.type == DeviceType::Threads)
+    device_label += "Threads";
+  else if (eid.type == DeviceType::SYCL)
+    device_label += "SYCL";
+  else if (eid.type == DeviceType::OpenACC)
+    device_label += "OpenACC";
+  else if (eid.type == DeviceType::Unknown)
+    device_label += "Unknown";
+  else
+    device_label += "Unknown to KokkosTools";


I wouldn't be opposed to pushing this part (or even the whole function) to Kokkos_Profiling_Interface.hpp. Getting string out of the device id doesn't seem to be specific to the kernel logger tool (also see https://github.com/kokkos/kokkos-tools/pull/265/files#diff-839f34fcb31addd9a48252bebf4d37cf674f6fcdd16539c7039813192674b9c0R165-R187).

Concur. I also have to maintain this function in Trilinos, so I would love to put it in Kokkos_Profiling_Interface.hpp.

If @crtrott doesn't object, I'll open a Kokkos PR with that.

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>

vlkale · 2024-10-17T15:07:31Z

@vlkale This is a debugging tool that fences all over the place. Host-side conditionals are basically irrelevant in terms of the performance hit the fences create.

True about fencing (aside: maybe in the future this tool could reduce fencing more, e.g., just allowing for global fencing to be turned off, if some Kokkos users want that). Stepping back though and forgetting about efficiency, I think I may be actually misunderstanding the purpose. I thought you are handling a case where the output of kernel-logger is not different across different Kokkos backends for a particular Kokkos application like LAMMPS, but also is non-deterministic for a given Kokkos backend. Did I get that right from "comparing two versions of the code with roughly the same flow, but some different kernel calls here and there. Specifically, code paths with and without UnifiedMemory." ?

As for the compile error, that's because the headers are no longer consistent with the headers in Kokkos. I'll add a fix to that in a second.

Thanks and I realized just now that this is the issue that Kokkos Tools Github Issue #275 brings up.

csiefer2 · 2024-10-17T16:06:32Z

I think I may be actually misunderstanding the purpose. I thought you are handling a case where the output of kernel-logger is not different across different Kokkos backends for a particular Kokkos application like LAMMPS, but also is non-deterministic for a given Kokkos backend. Did I get that right from "comparing two versions of the code with roughly the same flow, but some different kernel calls here and there. Specifically, code paths with and without UnifiedMemory." ?

I'm interested using the tool to track down cases in the case code flow is deterministic, but the answer is not due to an unintended race condition.

romintomasetti · 2024-10-17T16:56:12Z

+  using namespace Kokkos::Tools::Experimental;
+  std::string device_label("(");
+  ExecutionSpaceIdentifier eid = identifier_from_devid(deviceId);
+  if (eid.type == DeviceType::Serial)


This is a switch case.

romintomasetti · 2024-10-17T17:02:37Z

+    if (varVal) {
+      std::string v = std::string(varVal);
+      // default to false
+      if (v == "1" || v == "ON" || v == "on" || v == "TRUE" || v == "true" ||


Most of the time, it seems that this repo wants either 0 or 1 for the value of an environment variable.

Then use atoi.

I think it is important that all environment variables follow consistent conventions.

@romintomasetti This is a Tpetra-ism to allow it to take anything reasonable as an option. If @vlkale or @crtrott doesn't like it that's fine, but we've found the flexibility to be useful in Trilinos

romintomasetti · 2024-10-17T17:08:08Z

+      "KokkosP: Executing parallel-scan kernel on device %s with unique "
      "execution identifier %llu\n",
-      devID, (unsigned long long)(*kID));
+      deviceIdToString(devID).c_str(), (unsigned long long)(output));


Suggested change

deviceIdToString(devID).c_str(), (unsigned long long)(output));

deviceIdToString(devID).c_str(), suppressCounts() ? 0 : *kID);

That's twisted, but sure, why not? Co-authored-by: Tomasetti Romin <romin.tomasetti@gmail.com>

Kernel logger upgrades for better fence control

aecdb82

profiling/all: Updating from Kokkos develop

8358c9f

Signed-off-by: Chris Siefert <csiefer@sandia.gov>

All hail clang

62a39a1

Signed-off-by: Chris Siefert <csiefer@sandia.gov>

masterleinad reviewed Oct 17, 2024

View reviewed changes

csiefer2 and others added 2 commits October 17, 2024 08:36

Update debugging/kernel-logger/kp_kernel_logger.cpp

21d6359

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>

Update kp_kernel_logger.cpp

5f15867

romintomasetti suggested changes Oct 17, 2024

View reviewed changes

romintomasetti reviewed Oct 17, 2024

View reviewed changes

Comment thread debugging/kernel-logger/kp_kernel_logger.cpp

romintomasetti reviewed Oct 17, 2024

View reviewed changes

Comment thread debugging/kernel-logger/kp_kernel_logger.cpp Outdated

romintomasetti reviewed Oct 17, 2024

View reviewed changes

maartenarnst mentioned this pull request Oct 18, 2024

Compilation with Kokkos develop broken after Kokkos PR 7394 #275

Open

Update debugging/kernel-logger/kp_kernel_logger.cpp

520f74f

That's twisted, but sure, why not? Co-authored-by: Tomasetti Romin <romin.tomasetti@gmail.com>

maartenarnst mentioned this pull request Dec 16, 2024

fix(consistency with Kokkos): after Kokkos PR 7394 #279

Merged

vlkale added enhancement labels Jun 25, 2025

	deviceIdToString(devID).c_str(), (unsigned long long)(output));
	deviceIdToString(devID).c_str(), suppressCounts() ? 0 : *kID);

Conversation

csiefer2 commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csiefer2 commented Oct 16, 2024

Uh oh!

vlkale commented Oct 16, 2024

Uh oh!

csiefer2 commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csiefer2 commented Oct 16, 2024

Uh oh!

masterleinad Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

csiefer2 Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

csiefer2 Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vlkale commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csiefer2 commented Oct 17, 2024

Uh oh!

romintomasetti Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

romintomasetti Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

csiefer2 Nov 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

romintomasetti Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

csiefer2 commented Oct 16, 2024 •

edited

Loading

csiefer2 commented Oct 16, 2024 •

edited

Loading

vlkale commented Oct 17, 2024 •

edited

Loading