Kernel logger upgrades for better output and better code comparisons #276

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_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 <[email protected]>

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

Compiles just fine now...

Signed-off-by: Chris Siefert <[email protected]>

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

debugging/kernel-logger/kp_kernel_logger.cpp

+  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.

debugging/kernel-logger/kp_kernel_logger.cpp

Co-authored-by: Daniel Arndt <[email protected]>

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

debugging/kernel-logger/kp_kernel_logger.cpp

+  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

debugging/kernel-logger/kp_kernel_logger.cpp

+    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 · 2024-10-17T17:03:29Z

debugging/kernel-logger/kp_kernel_logger.cpp

+
+bool suppressCounts() {
+  static bool value       = [](){
+    const char* varVal = std::getenv("KOKKOS_TOOLS_LOGGER_SUPPRESS_COUNTS");


Please update your PR description in which you're currently mentioning KOKKOS_PROFILE_SUPPRESS_COUNTS.

romintomasetti · 2024-10-17T17:06:39Z

debugging/kernel-logger/kp_kernel_logger.cpp

      "execution identifier %llu\n",
-      devID, (unsigned long long)(*kID));
+      deviceIdToString(devID).c_str(), (unsigned long long)(output));


Wait... What if we have several devices ? For instance, several Cuda GPUs.

Seems this change is really not going in the direction of allowing devices to be distinguished.

I would be OKish with:

Suggested change

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

printf(

"KokkosP: Executing parallel-scan kernel on device %d (%s) with unique "

"execution identifier %llu\n",

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

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

debugging/kernel-logger/kp_kernel_logger.cpp

      "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);

Kernel logger upgrades for better fence control

aecdb82

profiling/all: Updating from Kokkos develop

8358c9f

Signed-off-by: Chris Siefert <[email protected]>

All hail clang

62a39a1

Signed-off-by: Chris Siefert <[email protected]>

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 <[email protected]>

Update kp_kernel_logger.cpp

5f15867

romintomasetti suggested changes Oct 17, 2024

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kernel logger upgrades for better output and better code comparisons #276

Kernel logger upgrades for better output and better code comparisons #276

csiefer2 commented Oct 16, 2024

csiefer2 commented Oct 16, 2024

vlkale commented Oct 16, 2024

csiefer2 commented Oct 16, 2024 •

edited

Loading

csiefer2 commented Oct 16, 2024

masterleinad Oct 17, 2024

csiefer2 Oct 17, 2024

csiefer2 Oct 17, 2024

vlkale commented Oct 17, 2024 •

edited

Loading

csiefer2 commented Oct 17, 2024

romintomasetti Oct 17, 2024

romintomasetti Oct 17, 2024

romintomasetti Oct 17, 2024

romintomasetti Oct 17, 2024

romintomasetti Oct 17, 2024

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

Kernel logger upgrades for better output and better code comparisons #276

Are you sure you want to change the base?

Kernel logger upgrades for better output and better code comparisons #276

Conversation

csiefer2 commented Oct 16, 2024

csiefer2 commented Oct 16, 2024

vlkale commented Oct 16, 2024

csiefer2 commented Oct 16, 2024 • edited Loading

csiefer2 commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlkale commented Oct 17, 2024 • edited Loading

csiefer2 commented Oct 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csiefer2 commented Oct 16, 2024 •

edited

Loading

vlkale commented Oct 17, 2024 •

edited

Loading