Add `likely` and `unlikely` attribute from C++20 to ref counting in views #6730

rgayatri23 · 2024-01-19T17:27:14Z

The PR is to address issue #6508.

The PR adds the following:

Macros to mark branches with likely and unlikely attribute from C++20 for reference counting of views.
A benchmark to understand the performance implications when using the attributes inside and outside a kernel.

…NCH_UNLIKELY macro.

cdaley · 2024-01-23T00:35:56Z

Thank you for adding this draft PR. Is there a reason for the specific KOKKOS_COMPILER_CLANG macro around the attribute? The likelihood attributes are legal C++20 and should be available in any compliant compiler, so the C++ version check should suffice.

rgayatri23 · 2024-01-23T18:36:56Z

Thank you for adding this draft PR. Is there a reason for the specific KOKKOS_COMPILER_CLANG macro around the attribute? The likelihood attributes are legal C++20 and should be available in any compliant compiler, so the C++ version check should suffice.

That's right. Fixed it.

cdaley · 2024-01-23T19:18:36Z

core/src/impl/Kokkos_SharedAlloc.hpp

+#if defined(KOKKOS_IMPL_REF_COUNT_BRANCH_LIKELY)
+#define KOKKOS_BRANCH_PROB [[likely]]
+#elif defined(KOKKOS_IMPL_REF_COUNT_BRANCH_UNLIKELY)
+#define KOKKOS_BRANCH_PROB [[unlikely]]


I also noticed that you need another else branch. This was not needed in the original code because it did not have a KOKKOS_ENABLE_CXX20 branch.

#else #define KOKKOS_BRANCH_PROB

rgayatri23 · 2024-01-23T19:33:33Z

core/src/impl/Kokkos_SharedAlloc.hpp

+#define KOKKOS_BRANCH_PROB [[unlikely]]
+#endif
+#else
+#define KOKKOS_BRANCH_PROB


I do have an else here. Do you mean something else @cdaley ?

core/src/impl/Kokkos_SharedAlloc.hpp

crtrott · 2024-02-01T01:15:15Z

core/src/impl/Kokkos_SharedAlloc.hpp

+#if defined(KOKKOS_ENABLE_CXX20)
+#if defined(KOKKOS_IMPL_REF_COUNT_BRANCH_LIKELY)
+#define KOKKOS_BRANCH_PROB [[likely]]
+#elif defined(KOKKOS_IMPL_REF_COUNT_BRANCH_UNLIKELY)
+#define KOKKOS_BRANCH_PROB [[unlikely]]
+#endif
+#endif


Is there any reason why not just do this:

Suggested change

#if defined(KOKKOS_ENABLE_CXX20)

#if defined(KOKKOS_IMPL_REF_COUNT_BRANCH_LIKELY)

#define KOKKOS_BRANCH_PROB [[likely]]

#elif defined(KOKKOS_IMPL_REF_COUNT_BRANCH_UNLIKELY)

#define KOKKOS_BRANCH_PROB [[unlikely]]

#endif

#endif

#if defined(KOKKOS_ENABLE_CXX20)

#define KOKKOS_BRANCH_PROB [[unlikely]]

#endif

crtrott · 2024-02-01T01:17:17Z

Basically either we do it or we don't. We could have an IMPL option to turn it off in case we get complaints, but there is no reason for us to switch back and forth. This will just cause issues down the road. Right now by default this is not on ever.

crtrott

I don't think we should do this this way. When we merge this by default the unlikely argument should be used. And MAYBE we have an impl option to turn it off. But if so there needs to be a cmake option.

…ews.

rgayatri23 · 2024-02-01T18:45:19Z

Followed @crtrott suggestion. Using [unlikely] attribute by default unless specified by a cake IMPL macro.

masterleinad · 2024-02-01T18:51:11Z

core/src/impl/Kokkos_SharedAlloc.hpp

+#if defined(KOKKOS_ENABLE_IMPL_REF_COUNT_BRANCH_LIKELY) && \
+    defined(KOKKOS_ENABLE_CXX20)
+#define KOKKOS_BRANCH_PROB [[likely]]
+#else
+#define KOKKOS_BRANCH_PROB [[unlikely]]
+#endif


This won't work. It should be

Suggested change

#if defined(KOKKOS_ENABLE_IMPL_REF_COUNT_BRANCH_LIKELY) && \

defined(KOKKOS_ENABLE_CXX20)

#define KOKKOS_BRANCH_PROB [[likely]]

#else

#define KOKKOS_BRANCH_PROB [[unlikely]]

#endif

#if defined(KOKKOS_ENABLE_CXX20)

#define KOKKOS_BRANCH_PROB [[unlikely]]

#else

#define KOKKOS_BRANCH_PROB

#endif

to conform to @crtrott's suggestion.

So we don't need a user provided option where they can opt for [likely] attribute?

Right. We are only considering adding [[unlikely]].

benchmarks/CMakeLists.txt

cdaley · 2024-02-05T17:51:01Z

benchmarks/copy_view_performance/Makefile

+CXX = clang++
+EXE = copy_view_performance.exe
+
+CXXFLAGS ?= -Ofast -DKOKKOS_IMPL_REF_COUNT_BRANCH_UNLIKELY


This macro should now be "KOKKOS_ENABLE_IMPL_REF_COUNT_BRANCH_UNLIKELY" to be the same as the macro in core/src/impl/Kokkos_SharedAlloc.hpp

crtrott · 2024-02-06T16:27:19Z

cmake/kokkos_enable_options.cmake

@@ -48,6 +48,7 @@ KOKKOS_ENABLE_OPTION(CUDA_LAMBDA ${CUDA_LAMBDA_DEFAULT} "Whether to allow lambda
 # resolved but we keep the option around a bit longer to be safe.
 KOKKOS_ENABLE_OPTION(IMPL_CUDA_MALLOC_ASYNC ON  "Whether to enable CudaMallocAsync (requires CUDA Toolkit 11.2)")
 KOKKOS_ENABLE_OPTION(IMPL_NVHPC_AS_DEVICE_COMPILER OFF "Whether to allow nvc++ as Cuda device compiler")
+KOKKOS_ENABLE_OPTION(IMPL_REF_COUNT_BRANCH_UNLIKELY ON "Turn on `likely` attribute from C++20 when reference counting view copies")


Suggested change

KOKKOS_ENABLE_OPTION(IMPL_REF_COUNT_BRANCH_UNLIKELY ON "Turn on `likely` attribute from C++20 when reference counting view copies")

KOKKOS_ENABLE_OPTION(IMPL_REF_COUNT_BRANCH_UNLIKELY ON "Turn on `unlikely` attribute from C++20 when reference counting view copies")

crtrott · 2024-02-06T16:28:00Z

core/src/impl/Kokkos_SharedAlloc.hpp

@@ -24,6 +24,15 @@
 #include <cstdint>
 #include <string>

+// Use likely, unlikely attributes from C++20 to improve performance of


Suggested change

// Use likely, unlikely attributes from C++20 to improve performance of

// Use unlikely attribute from C++20 to improve performance of

Add KOKKOS_IMPL_REF_COUNT_BRANCH_LIKELY and KOKKOS_IMPL_REF_COUNT_BRA…

525f416

…NCH_UNLIKELY macro.

rgayatri23 added Enhancement Improve existing capability; will potentially require voting InDevelop Enhancement, fix, etc. has been merged into the develop branch; labels Jan 19, 2024

rgayatri23 self-assigned this Jan 19, 2024

masterleinad marked this pull request as draft January 22, 2024 14:26

Remove compiler macro for likely attribute.

2de2248

cdaley reviewed Jan 23, 2024

View reviewed changes

rgayatri23 commented Jan 23, 2024

View reviewed changes

masterleinad reviewed Jan 23, 2024

View reviewed changes

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

Fix macro.

4b70d72

rgayatri23 marked this pull request as ready for review January 29, 2024 07:39

crtrott reviewed Feb 1, 2024

View reviewed changes

crtrott requested changes Feb 1, 2024

View reviewed changes

Add cmake macro for likely attribute during reference counting for vi…

b8a15b0

…ews.

masterleinad reviewed Feb 1, 2024

View reviewed changes

dalg24 reviewed Feb 1, 2024

View reviewed changes

benchmarks/CMakeLists.txt Show resolved Hide resolved

Remove likely option.

4ebd157

cdaley reviewed Feb 5, 2024

View reviewed changes

crtrott reviewed Feb 6, 2024

View reviewed changes

ajpowelsnl mentioned this pull request Feb 28, 2024

Release Themes for 2024 #6804

Open

Rahulkumar Gayatri added 2 commits March 4, 2024 14:01

Add CMakeLists for copy_view_performance benchmark.

4684c11

Edit macro name in Makefile.

58c7eda

ajpowelsnl mentioned this pull request Mar 25, 2024

Add likelihood attribute in Kokkos source to improve performance with LLVM compilers #6508

Open

masterleinad removed the InDevelop Enhancement, fix, etc. has been merged into the develop branch; label Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `likely` and `unlikely` attribute from C++20 to ref counting in views #6730

Add `likely` and `unlikely` attribute from C++20 to ref counting in views #6730

rgayatri23 commented Jan 19, 2024 •

edited

cdaley commented Jan 23, 2024

rgayatri23 commented Jan 23, 2024

cdaley Jan 23, 2024

rgayatri23 Jan 23, 2024

crtrott Feb 1, 2024

crtrott commented Feb 1, 2024

crtrott left a comment

rgayatri23 commented Feb 1, 2024

masterleinad Feb 1, 2024

rgayatri23 Feb 2, 2024

masterleinad Feb 2, 2024

cdaley Feb 5, 2024

crtrott Feb 6, 2024

crtrott Feb 6, 2024

	KOKKOS_ENABLE_OPTION(IMPL_REF_COUNT_BRANCH_UNLIKELY ON "Turn on `likely` attribute from C++20 when reference counting view copies")
	KOKKOS_ENABLE_OPTION(IMPL_REF_COUNT_BRANCH_UNLIKELY ON "Turn on `unlikely` attribute from C++20 when reference counting view copies")

	// Use likely, unlikely attributes from C++20 to improve performance of
	// Use unlikely attribute from C++20 to improve performance of

Add likely and unlikely attribute from C++20 to ref counting in views #6730

Are you sure you want to change the base?

Add likely and unlikely attribute from C++20 to ref counting in views #6730

Conversation

rgayatri23 commented Jan 19, 2024 • edited

cdaley commented Jan 23, 2024

rgayatri23 commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crtrott commented Feb 1, 2024

crtrott left a comment

Choose a reason for hiding this comment

rgayatri23 commented Feb 1, 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

Choose a reason for hiding this comment

Add `likely` and `unlikely` attribute from C++20 to ref counting in views #6730

Add `likely` and `unlikely` attribute from C++20 to ref counting in views #6730

rgayatri23 commented Jan 19, 2024 •

edited