Implement Kokkos::printf #6083

masterleinad · 2023-04-26T21:49:00Z

Supersedes #6061. This pull request implements Kokkos::printf as discussed.
After trying a bunch of implementations (just wrap KOKKOS_IMPL_DO_NOT_USE_PRINTF, define a separate function that calls printf), it seems that importing printf into the Kokkos namespace is the only option that works.

In my opinion, it's a good idea to keep the KOKKOS_IMPL_DO_NOT_USE_PRINTF macro around a little longer to allow some users to transition.

masterleinad · 2023-04-28T12:55:42Z

Retest this please.

masterleinad · 2023-05-09T17:29:45Z

NVHPC seems to error out when we are not using a string literal for the formatting string.

cz4rs

Looks good to me.

core/src/impl/Kokkos_Printf.hpp

dalg24 · 2023-06-29T05:41:32Z

core/unit_test/TestPrintf.hpp

+#endif
+
+  int backup_no, new_no;
+  fflush(stdout);


Any particular reason why you are not using ::testing::internal::CaptureStdout()
It is not part of the GTest API but we already use it elsewhere and if/when we get into trouble for it we can encapsulate and replace.

I didn't quite realize that we are already reaching into someone else's internal namespace. Sure, we can try that instead. It's just for testing anyway.

masterleinad · 2023-06-29T16:52:22Z

core/src/impl/Kokkos_Printf.hpp

+#elif defined(KOKKOS_COMPILER_NVHPC)  // FIXME_NVHPC
+template <typename... Args>
+KOKKOS_FUNCTION int printf(const char* format, Args... args) {
+  ::printf(format, args...);


Returning a value here doesn't work with NVHPC.

Did we discuss whether we should be providing a checked version or return anything at all?
By that I mean, should we attempt to cause program termination when we detect than an error occurred?
Should we return void instead of "passing through" the return value of printf as currently proposed?

I don't necessarily have an issue with what is proposed but I do want to make sure we considered alternatives.

I think it's good to stay close to what printf does since that's what people are used to (even though I doubt many people use the return value anyway).

dalg24 · 2023-06-30T09:50:23Z

core/src/impl/Kokkos_Printf.hpp

Did we discuss whether that should be a private or public header?

No, my intuition was to do the same as for core/src/impl/Kokko_Error.hpp (and it's easier to go from "private" to "public" than the other way around).

ldh4 · 2023-07-05T17:30:27Z

core/src/impl/Kokkos_Printf.hpp

+template <typename... Args>
+KOKKOS_FUNCTION int printf(const char* format, Args... args) {
+  ::printf(format, args...);
+  return 1;


In case anyone is using the return value of printf for some diagnostic, I think this should return some other value than 1 to avoid confusion. But since negative value is reserved to indicate errors for standard ::printf, perhaps 0 is the neutral option.

I was choosing 1 because that is what I get for most GPU backends and it's fairly difficult to make printf fail anyway.

dalg24 · 2023-07-05T21:40:21Z

core/src/impl/Kokkos_Printf.hpp

+// We are forwarding the format string to printf but gcc and clang warn about
+// the format string not being a literal.
+#if defined(__GNUC__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wformat-security"
+#endif


I don't get this one. Can you share a godbolt link?

I pushed commit to remove these lines and you can see the CI failing with these warnings.

Also, see https://godbolt.org/z/bdM4ozM9r where I found a different workaround when there are no arguments for the format string.

masterleinad · 2023-07-06T17:19:30Z

Failures in CUDA-11.7-NVHPC

[ 92%] Building CXX object core/unit_test/CMakeFiles/Kokkos_CoreUnitTest_Cuda1.dir/cuda/TestCuda_RangePolicyConstructors.cpp.o
NVC++-S-0000-Internal compiler error. extractor: bad opc       0  (/var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsMoveBackward.cpp: 216)
NVC++-S-0000-Internal compiler error. extractor: bad opc       0  (/var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsMoveBackward.cpp: 216)
NVC++-S-0000-Internal compiler error. extractor: bad opc       0  (/var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsMoveBackward.cpp: 216)
NVC++-S-0000-Internal compiler error. extractor: bad opc       0  (/var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsMoveBackward.cpp: 216)
NVC++-S-0000-Internal compiler error. extractor: bad opc       0  (/var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsMoveBackward.cpp: 216)
NVC++-S-0000-Internal compiler error. extractor: bad opc       0  (/var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsMoveBackward.cpp: 216)
NVC++-S-0000-Internal compiler error. extractor: bad opc       0  (/var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsMoveBackward.cpp: 216)
NVC++-S-0000-Internal compiler error. extractor: bad opc       0  (/var/jenkins/workspace/Kokkos/algorithms/unit_tests/TestStdAlgorithmsMoveBackward.cpp: 216)
[ 92%] Building CXX object core/unit_test/CMakeFiles/Kokkos_CoreUnitTest_OpenMP.dir/openmp/TestOpenMP_ViewCopy_a.cpp.o
NVC++/x86-64 Linux 22.9-0: compilation completed with severe errors

are unrelated.

dalg24 · 2023-07-06T18:02:52Z

Do these show on other builds?

masterleinad · 2023-07-06T18:06:19Z

Do these show on other builds?

I have seen that in a couple of other builds.

core/src/impl/Kokkos_Printf.hpp

core/unit_test/TestPrintf.hpp

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

masterleinad mentioned this pull request Apr 26, 2023

Import sycl::ext::oneapi::experimental::printf into global namespace #6061

Closed

masterleinad force-pushed the kokkos_printf branch from 82e8086 to 97c2c21 Compare April 26, 2023 21:51

masterleinad mentioned this pull request May 1, 2023

Add documentation for Kokkos::printf kokkos/kokkos-core-wiki#369

Merged

masterleinad force-pushed the kokkos_printf branch from 477ef77 to 8f67e27 Compare May 3, 2023 17:56

masterleinad marked this pull request as ready for review June 26, 2023 15:47

cz4rs approved these changes Jun 26, 2023

View reviewed changes

cz4rs reviewed Jun 26, 2023

View reviewed changes

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

Rombur approved these changes Jun 27, 2023

View reviewed changes

masterleinad force-pushed the kokkos_printf branch from 4bd9a5e to 4537fee Compare June 28, 2023 13:53

masterleinad marked this pull request as draft June 28, 2023 15:12

masterleinad force-pushed the kokkos_printf branch 2 times, most recently from 58eea6c to 1b71a4f Compare June 28, 2023 21:11

dalg24 reviewed Jun 29, 2023

View reviewed changes

masterleinad marked this pull request as ready for review June 29, 2023 16:51

masterleinad commented Jun 29, 2023

View reviewed changes

dalg24 reviewed Jun 30, 2023

View reviewed changes

ldh4 reviewed Jul 5, 2023

View reviewed changes

masterleinad force-pushed the kokkos_printf branch from 3cb8f35 to 55948b4 Compare July 5, 2023 20:44

dalg24 reviewed Jul 5, 2023

View reviewed changes

dalg24 reviewed Jul 6, 2023

View reviewed changes

core/src/impl/Kokkos_Printf.hpp Show resolved Hide resolved

dalg24 reviewed Jul 6, 2023

View reviewed changes

core/src/impl/Kokkos_Printf.hpp Show resolved Hide resolved

dalg24 reviewed Jul 6, 2023

View reviewed changes

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

masterleinad added 2 commits July 7, 2023 10:16

Implement Kokkos::printf

eba9d89

Use Kokkos::printf

4474e8e

masterleinad force-pushed the kokkos_printf branch from c3a0409 to 4474e8e Compare July 7, 2023 14:17

dalg24 reviewed Jul 7, 2023

View reviewed changes

core/unit_test/TestPrintf.hpp Outdated Show resolved Hide resolved

Update core/unit_test/TestPrintf.hpp

69dfe8f

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

dalg24 merged commit 597bc36 into kokkos:develop Jul 8, 2023
28 checks passed

masterleinad mentioned this pull request Jul 14, 2023

CHANGELOG: 4.2.0 #6197

Closed

masterleinad mentioned this pull request Aug 18, 2023

Replace KOKKOS_IMPL_DO_NOT_USE_PRINTF with Kokkos::printf kokkos/kokkos-kernels#1946

Closed

masterleinad mentioned this pull request Oct 12, 2023

Promote Kokkos_Printf.hpp to public include #6506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Kokkos::printf #6083

Implement Kokkos::printf #6083

masterleinad commented Apr 26, 2023 •

edited

masterleinad commented Apr 28, 2023

masterleinad commented May 9, 2023

cz4rs left a comment

dalg24 Jun 29, 2023

masterleinad Jun 29, 2023

masterleinad Jun 29, 2023

dalg24 Jun 30, 2023

masterleinad Jun 30, 2023

dalg24 Jun 30, 2023

masterleinad Jun 30, 2023

ldh4 Jul 5, 2023

masterleinad Jul 5, 2023

dalg24 Jul 5, 2023

masterleinad Jul 6, 2023

masterleinad Jul 6, 2023

masterleinad commented Jul 6, 2023

dalg24 commented Jul 6, 2023

masterleinad commented Jul 6, 2023

Implement Kokkos::printf #6083

Implement Kokkos::printf #6083

Conversation

masterleinad commented Apr 26, 2023 • edited

masterleinad commented Apr 28, 2023

masterleinad commented May 9, 2023

cz4rs left a 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

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

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

masterleinad commented Jul 6, 2023

dalg24 commented Jul 6, 2023

masterleinad commented Jul 6, 2023

masterleinad commented Apr 26, 2023 •

edited