OpenMPTarget: Enable Cray compiler for the OpenMPTarget backend. #5889

rgayatri23 · 2023-02-17T22:45:28Z

The PR does the following :

Adds a macro for Clang based Cray compiler as KOKKOS_COMPILER_CRAY_CLANG
Enables appropriate flags for OpenMPTarget backend with Cray compiler in Makefile.kokkos and in cmake/kokkos_arch.cmake.
Disable unit tests that do not compile or fail at runtime with the OpenMPTarget backend and Cray compiler.

crtrott

Some small changes needed (comments and indentation)

crtrott · 2023-02-18T22:50:43Z

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Instance.cpp

@@ -93,13 +93,16 @@ void OpenMPTargetInternal::impl_initialize() {

  // FIXME_OPENMPTARGET:  Only fix the number of teams for NVIDIA architectures
  // from Pascal and upwards.
+  // Cray compiler did not yet implement omp_set_num_teams.


Add a FIXME for Cray

crtrott · 2023-02-18T22:51:28Z

core/unit_test/CMakeLists.txt

@@ -353,6 +353,12 @@ if(Kokkos_ENABLE_OPENMPTARGET)
    ${CMAKE_CURRENT_BINARY_DIR}/openmptarget/TestOpenMPTarget_Other.cpp
    ${CMAKE_CURRENT_BINARY_DIR}/openmptarget/TestOpenMPTarget_TeamReductionScan.cpp
    ${CMAKE_CURRENT_BINARY_DIR}/openmptarget/TestOpenMPTarget_WorkGraph.cpp
+    # The following tests fail at compile time when the OpenMPTarget backend is enabled with the Cray compiler.


Add a FIXME comment also this is all kinds of atomic stuff - this could be a real issue. As if compare/exchange doesn't work properly.

crtrott · 2023-02-18T22:52:08Z

core/unit_test/TestTeamMDRange.hpp

+  if (std::is_same_v<TEST_EXECSPACE, Kokkos::Experimental::OpenMPTarget>)
+    GTEST_SKIP() << "Cray compiler fails correctness at runtime with the "
+                    "OpenMPTarget backend.";
+#endif


Add a FIXME comment

crtrott · 2023-02-18T22:52:28Z

core/unit_test/TestTeamMDRange.hpp

+  if (std::is_same_v<TEST_EXECSPACE, Kokkos::Experimental::OpenMPTarget>)
+    GTEST_SKIP() << "Cray compiler fails correctness at runtime with the "
+                    "OpenMPTarget backend.";
+#endif


Add a FIXME comment something with "Cray" in the same line too.

crtrott · 2023-02-18T22:52:53Z

cmake/kokkos_arch.cmake

@@ -680,12 +680,18 @@ ENDIF()
 IF (KOKKOS_ENABLE_OPENMPTARGET)
  SET(CLANG_CUDA_ARCH ${KOKKOS_CUDA_ARCH_FLAG})
  IF (CLANG_CUDA_ARCH)
+      IF(KOKKOS_CLANG_IS_CRAY)


fix identation.

dalg24

Can you comment about the choice to distinguish between the legacy and the Clang-based Cray compilers?

Also with Intel we do not define KOKKOS_COMPILER_CLANG

kokkos/core/src/Kokkos_Macros.hpp

Lines 151 to 152 in 948c6c6

    
           #if defined(__clang__) && !defined(KOKKOS_COMPILER_INTEL) 
        
           #define KOKKOS_COMPILER_CLANG \

Did you mean to define both KOKKOS_COMPILER_CRAYCLANG and KOKKOS_COMPILER_CLANG?

rgayatri23 · 2023-02-20T06:35:52Z

The KOKKOS_COMPILER_CLANG gets defined in the lines above since its already checking for __clang__ .
I added a comment to differentiate between classical cray and clang based cray compilers. Also added version number math for KOKKOS_COMPILER_CRAYCLANG similar to the clang compiler.

With respect to Intel compiler, I assume the same __clang__ will be defined which will define the respective macro (although I am not sure about it).

dalg24 · 2023-02-20T13:27:48Z

With respect to Intel compiler, I assume the same __clang__ will be defined which will define the respective macro (although I am not sure about it).

Look at the code snippet I referenced. We define KOKKOS_COMPILER_CLANG only if KOKKOS_COMPILER_INTEL is not defined.

dalg24 · 2023-02-20T13:32:57Z

I added a comment to differentiate between classical cray and clang based cray compilers.

I am asking about clarifying why we want to distinguish them in the first place. I am not necessary suggesting we should not distinguish them. I just observing we don't do it for Intel and I am wondering whether you thought about it and why you decided to do it this way.

masterleinad · 2023-02-20T13:22:33Z

Makefile.kokkos

+	# Cray compiler only needs fopenmp flag for target offload.
+  ifeq ($(KOKKOS_INTERNAL_COMPILER_CRAY_CLANG), 1)
+	  KOKKOS_INTERNAL_OPENMPTARGET_FLAG := -DKOKKOS_WORKAROUND_OPENMPTARGET_CLANG -fopenmp 
+  else
+	  KOKKOS_INTERNAL_OPENMPTARGET_FLAG := -DKOKKOS_WORKAROUND_OPENMPTARGET_CLANG -fopenmp -fopenmp=libomp -Wno-openmp-mapping
+       KOKKOS_INTERNAL_OPENMPTARGET_LIB := -lomptarget
+  endif


Suggested change

# Cray compiler only needs fopenmp flag for target offload.

ifeq ($(KOKKOS_INTERNAL_COMPILER_CRAY_CLANG), 1)

KOKKOS_INTERNAL_OPENMPTARGET_FLAG := -DKOKKOS_WORKAROUND_OPENMPTARGET_CLANG -fopenmp

else

KOKKOS_INTERNAL_OPENMPTARGET_FLAG := -DKOKKOS_WORKAROUND_OPENMPTARGET_CLANG -fopenmp -fopenmp=libomp -Wno-openmp-mapping

KOKKOS_INTERNAL_OPENMPTARGET_LIB := -lomptarget

endif

# Cray compiler only needs fopenmp flag for target offload.

ifeq ($(KOKKOS_INTERNAL_COMPILER_CRAY_CLANG), 1)

KOKKOS_INTERNAL_OPENMPTARGET_FLAG := -DKOKKOS_WORKAROUND_OPENMPTARGET_CLANG -fopenmp

else

KOKKOS_INTERNAL_OPENMPTARGET_FLAG := -DKOKKOS_WORKAROUND_OPENMPTARGET_CLANG -fopenmp -fopenmp=libomp -Wno-openmp-mapping

KOKKOS_INTERNAL_OPENMPTARGET_LIB := -lomptarget

endif

masterleinad · 2023-02-20T13:26:19Z

Makefile.kokkos

+	# Not necessary for the Cray compiler.
+  ifeq ($(KOKKOS_INTERNAL_COMPILER_CRAY_CLANG), 0)
  ifeq ($(KOKKOS_INTERNAL_COMPILER_CLANG), 1)
    KOKKOS_INTERNAL_CUDA_ARCH_FLAG=-fopenmp-targets=nvptx64 -Xopenmp-target -march
  endif
+  endif


Suggested change

# Not necessary for the Cray compiler.

ifeq ($(KOKKOS_INTERNAL_COMPILER_CRAY_CLANG), 0)

ifeq ($(KOKKOS_INTERNAL_COMPILER_CLANG), 1)

KOKKOS_INTERNAL_CUDA_ARCH_FLAG=-fopenmp-targets=nvptx64 -Xopenmp-target -march

endif

endif

# Not necessary for the Cray compiler.

ifeq ($(KOKKOS_INTERNAL_COMPILER_CRAY_CLANG), 0)

ifeq ($(KOKKOS_INTERNAL_COMPILER_CLANG), 1)

KOKKOS_INTERNAL_CUDA_ARCH_FLAG=-fopenmp-targets=nvptx64 -Xopenmp-target -march

endif

endif

masterleinad · 2023-02-20T13:28:22Z

Makefile.kokkos

+# Do not add this flag it its the cray compiler.
+ifeq ($(KOKKOS_INTERNAL_COMPILER_CRAY_CLANG), 0)


Please also indent accordingly.
So the Cray compiler doesn't take any architecture flags at all?!
Does this also apply if compiling for the Cuda backend?

No Cray compiler does not take any architecture flags. I was also surprised by that update. Why would we compile Cuda backend with Cray compiler, do you mean using Cray Wrappers? Cuda backend points to nvcc_wrapper anyways right?

masterleinad · 2023-02-20T13:35:01Z

core/src/Kokkos_Macros.hpp

@@ -153,6 +153,13 @@
  __clang_major__ * 100 + __clang_minor__ * 10 + __clang_patchlevel__
 #endif

+// Macro for the Cray compiler based on clang.
+// Different from classical Cray wihch is defined above.


Suggested change

// Different from classical Cray wihch is defined above.

// Different from classical Cray which is defined above.

Is it a problem that the compiler now identifies as both CLANG and CRAYCLANG?

rgayatri23 · 2023-02-20T18:12:35Z

With respect to Intel compiler, I assume the same __clang__ will be defined which will define the respective macro (although I am not sure about it).

Look at the code snippet I referenced. We define KOKKOS_COMPILER_CLANG only if KOKKOS_COMPILER_INTEL is not defined.

Ah yes thanks. I missed the 2nd condition on that line.

rgayatri23 · 2023-02-20T18:14:08Z

I added a comment to differentiate between classical cray and clang based cray compilers.

I am asking about clarifying why we want to distinguish them in the first place. I am not necessary suggesting we should not distinguish them. I just observing we don't do it for Intel and I am wondering whether you thought about it and why you decided to do it this way.

Added a comment on the need for a new macro since the newer Clang based Cray compilers have a mature OpenMP offload implementation but we still want the older Classical Cray macro for the host if that is being used somewhere.

rgayatri23 · 2023-02-20T18:19:53Z

@masterleinad has a good point which even @dalg24 is referring to, that now we define both Clang and CRAYCLANG and that might be an issue. I can disable KOKKOS_COMPILER_CLANG macro if KOKKOS_COMPILER_CRAYCLANG is defined, which I suppose is being done with Intel compilers. Any suggestions? I do not have an opinion on that right now.

masterleinad · 2023-05-10T20:02:31Z

@rgayatri23 This needs rebasing.

masterleinad · 2023-05-11T12:23:35Z

cmake/kokkos_arch.cmake

+    IF(KOKKOS_CLANG_IS_CRAY)
+      COMPILER_SPECIFIC_FLAGS(
+      Cray -fopenmp
+    )
+    ELSE()
+      STRING(REPLACE "sm_" "cc" NVHPC_CUDA_ARCH ${CLANG_CUDA_ARCH})
+      COMPILER_SPECIFIC_FLAGS(
      Clang -Xopenmp-target -march=${CLANG_CUDA_ARCH} -fopenmp-targets=nvptx64
      NVHPC -gpu=${NVHPC_CUDA_ARCH}
    )
+    ENDIF()


I know that most of it pre-existed but could you fix the indentation here in a sensible way?
Like that the closing ")" is at least as indented as the opening one.

Getting the closing bracket in the same level as the opening one looks weird, I tried to put the opening one in the next line but that does not match the style in the rest of the file. I indented the lines inside the brackets to make the nesting look obvious. Let me know if thats acceptable.

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Exec.cpp

core/src/OpenMPTarget/Kokkos_OpenMPTarget_ParallelFor_Team.hpp

core/src/OpenMPTarget/Kokkos_OpenMPTarget_Parallel_Common.hpp

masterleinad

Otherwise, this looks Ok to me.

cmake/kokkos_arch.cmake

masterleinad

Thanks for bearing with me! 🙂

Requested changes have been addressed.

…kos#5889) * OpenMPTarget: Enable Cray compiler for the OpenMPTarget backend. * OpenMPTarget: Adding a fixme for Cray compiler. * OpenMPTarget: Add fixme for cray compiler. * OpenMPTarget: fix indentation. --------- Co-authored-by: Rahulkumar Gayatri <rgayatri@lbl.gov>

crtrott previously requested changes Feb 18, 2023

View reviewed changes

dalg24 reviewed Feb 19, 2023

View reviewed changes

masterleinad reviewed Feb 20, 2023

View reviewed changes

rgayatri23 mentioned this pull request Apr 11, 2023

unsupported option '-fopenmp-targets=' for language mode 'HIP' #6044

Closed

rgayatri23 mentioned this pull request Apr 20, 2023

Define KOKKOS_COMPILER_INTEL_LLVM and only define at most one KOKKOS_COMPILER* macro #5906

Merged

Rahulkumar Gayatri added 2 commits May 10, 2023 19:27

OpenMPTarget: Enable Cray compiler for the OpenMPTarget backend.

0dafe27

OpenMPTarget: Adding a fixme for Cray compiler.

3b26a93

rgayatri23 force-pushed the OpenMPTarget_cray_compiler branch from 8fc7f7f to 3b26a93 Compare May 11, 2023 03:17

masterleinad reviewed May 11, 2023

View reviewed changes

OpenMPTarget: Add fixme for cray compiler.

ae7fd9f

masterleinad reviewed May 11, 2023

View reviewed changes

cmake/kokkos_arch.cmake Outdated Show resolved Hide resolved

OpenMPTarget: fix indentation.

f275e72

masterleinad approved these changes May 11, 2023

View reviewed changes

dalg24 approved these changes May 19, 2023

View reviewed changes

dalg24 merged commit b2645f8 into kokkos:develop May 19, 2023
28 checks passed

ndellingwood mentioned this pull request May 22, 2023

Trilinos nightly configuration fails with Kokkos' develop branch #6152

Closed

nmm0 mentioned this pull request Jun 16, 2023

CHANGELOG: 4.1.0 #5902

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenMPTarget: Enable Cray compiler for the OpenMPTarget backend. #5889

OpenMPTarget: Enable Cray compiler for the OpenMPTarget backend. #5889

rgayatri23 commented Feb 17, 2023

crtrott left a comment

crtrott Feb 18, 2023

crtrott Feb 18, 2023

crtrott Feb 18, 2023

crtrott Feb 18, 2023

crtrott Feb 18, 2023

crtrott Feb 18, 2023

dalg24 left a comment

rgayatri23 commented Feb 20, 2023

dalg24 commented Feb 20, 2023

dalg24 commented Feb 20, 2023

masterleinad Feb 20, 2023

masterleinad Feb 20, 2023

masterleinad Feb 20, 2023

rgayatri23 Feb 20, 2023

masterleinad Feb 20, 2023

rgayatri23 commented Feb 20, 2023

rgayatri23 commented Feb 20, 2023

rgayatri23 commented Feb 20, 2023 •

edited

masterleinad commented May 10, 2023

masterleinad May 11, 2023

rgayatri23 May 11, 2023

masterleinad left a comment

masterleinad left a comment

	#if defined(__clang__) && !defined(KOKKOS_COMPILER_INTEL)
	#define KOKKOS_COMPILER_CLANG \

		# Do not add this flag it its the cray compiler.
		ifeq ($(KOKKOS_INTERNAL_COMPILER_CRAY_CLANG), 0)

	// Different from classical Cray wihch is defined above.
	// Different from classical Cray which is defined above.

OpenMPTarget: Enable Cray compiler for the OpenMPTarget backend. #5889

OpenMPTarget: Enable Cray compiler for the OpenMPTarget backend. #5889

Conversation

rgayatri23 commented Feb 17, 2023

crtrott 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

dalg24 left a comment

Choose a reason for hiding this comment

rgayatri23 commented Feb 20, 2023

dalg24 commented Feb 20, 2023

dalg24 commented Feb 20, 2023

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

rgayatri23 commented Feb 20, 2023

rgayatri23 commented Feb 20, 2023

rgayatri23 commented Feb 20, 2023 • edited

masterleinad commented May 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masterleinad left a comment

Choose a reason for hiding this comment

masterleinad left a comment

Choose a reason for hiding this comment

rgayatri23 commented Feb 20, 2023 •

edited