SYCL in homme #6569

oksanaguba · 2024-08-28T18:04:42Z

Changes needed for spot/aurora: sycl kokkos spaces, temporary TP changes (TP init will be sorted out later), switch booleans to ints in F<-->xx interfaces.

[bfb]

Conflicts: components/homme/CMakeLists.txt components/homme/src/share/gllfvremap_mod.F90 components/homme/test_execs/share_kokkos_ut/CMakeLists.txt

oksanaguba · 2024-08-29T20:53:18Z

@bartgol or @ambrad in this branch i revived possibility to use external kokkos build instead of pointing ekat to a new kokkos version. i still prefer to have such option, but do you object to it?

ambrad · 2024-08-30T04:51:23Z

@oksanaguba is that for standalone Homme? I suspect that's an easier case for supporting an external Kokkos build than the CIME case. In any case, it's fine with me; I believe the option was lost by accident.

bartgol · 2024-09-03T20:46:47Z

@oksanaguba You can tell cmake to use a prebuilt Kokkos, even with the current infrastructure. You should be able to do it via either an env or a CMake variable. E.g., via env var:

export Kokkos_ROOT=/home/lbertag/workdir/libs/ekat/ekat-install/branch

cmake <options> /path/to/source

For me this gives the output:

-- Looking for a Kokkos installation ...
-- Looking for a Kokkos installation ... FOUND
--   Kokkos_DIR: /home/lbertag/workdir/libs/ekat/ekat-install/branch/lib64/cmake/Kokkos

The same should be achievable by setting the CMake var Kokkos_ROOT (or it may be Kokkos_DIR in that case, I'm not sure). Either way, you always point to the install root folder, of course.

Conflicts: components/homme/CMakeLists.txt components/homme/test_execs/share_kokkos_ut/CMakeLists.txt

oksanaguba · 2024-09-05T19:27:58Z

Ready for review, please, @bartgol and @ambrad (i will run tests meanwhile).

bartgol

I am confused by two things:

the linker language for SUNSPOT
all the bool conversions to int

I guess there may be some issues with bool on SYCL device (not sure if that's the case, but I can see something like that happening), so it's ok to create an int local var from a bool. But the rest of the infrastructure should use bool/logical for stuff that is conceptually a boolean.

bartgol · 2024-09-05T21:15:09Z

components/homme/cmake/HommeMacros.cmake

-  SET_TARGET_PROPERTIES(${execName} PROPERTIES LINKER_LANGUAGE Fortran)
+
+  if(SUNSPOT_MACHINE)
+    SET_TARGET_PROPERTIES(${execName} PROPERTIES LINKER_LANGUAGE CXX)


This puzzles me. The linker language should always be the one of the main entry point (in this case, F90). Why do we need this?

bartgol · 2024-09-05T21:15:35Z

components/homme/cmake/machineFiles/aurora-aot.cmake

@@ -0,0 +1,64 @@
+#module restore


I'm curious: what does "-aot" stand for?

ahead of time https://www.intel.com/content/www/us/en/docs/dpcpp-cpp-compiler/developer-guide-reference/2023-0/ahead-of-time-compilation.html

bartgol · 2024-09-05T21:16:19Z

components/homme/cmake/machineFiles/aurora-jit.cmake

@@ -0,0 +1,58 @@
+#module restore


And "-jit"? I doubt it is "Just In Time"?

it is "just in time" https://www.intel.com/content/www/us/en/docs/oneapi/optimization-guide-gpu/2023-0/just-in-time-compilation-in-sycl.html

bartgol · 2024-09-05T21:25:26Z

components/homme/src/share/cxx/GllFvRemap.cpp

@@ -16,7 +16,7 @@
 namespace Homme {

 void init_gllfvremap_c (int nelemd, int np, int nf, int nf_max,
-                        bool theta_hydrostatic_mode,


I'm assuming all these changes are b/c there's a problem getting bool values in a device lambda on SYCL, or something like that?

If that's the case, I would keep bool everywhere, and simply convert to int in the C++ structures that do need an int on SYCL. That would allow to leave lots of other files (including the F90 files) unchanged.

bartgol · 2024-09-05T21:29:19Z

components/homme/src/share/cxx/GllFvRemapImpl.cpp

@@ -395,7 +395,7 @@ ::run_dyn_to_fv_phys (const int timeidx, const Phys1T& ps, const Phys1T& phis, c
  const auto hvcoord = m_hvcoord;

  const bool use_moisture = m_data.use_moisture;
-  const bool theta_hydrostatic_mode = m_data.theta_hydrostatic_mode;
+  const int theta_hydrostatic_mode = m_data.theta_hydrostatic_mode;


I believe this is the only place where you need this to be an int, right? I see it used inside the KOKKOS_LAMBDA below. Then, you can keep theta_hydrostatic_mode as a bool everywhere, and just use an int for the local var here.

bartgol · 2024-09-05T21:32:50Z

components/homme/src/theta-l_kokkos/cxx/HyperviscosityFunctorImpl.cpp

 #else
-  m_process_nh_vars = !params.theta_hydrostatic_mode;
+  //m_process_nh_vars = !params.theta_hydrostatic_mode;
+  if (params.theta_hydrostatic_mode){


You can just do

m_process_nh_vars = static_cast<int>(params.theta_hydrostatic_mode);

Faster to read, and makes your intent of simply changing the type clearer.

bartgol · 2024-09-05T21:34:17Z

components/homme/src/theta-l_kokkos/cxx/cxx_f90_interface_theta.cpp

@@ -43,12 +43,13 @@ void init_simulation_params_c (const int& remap_alg, const int& limiter_option,
                               const Real& nu, const Real& nu_p, const Real& nu_q, const Real& nu_s, const Real& nu_div, const Real& nu_top,
                               const int& hypervis_order, const int& hypervis_subcycle, const int& hypervis_subcycle_tom,
                               const double& hypervis_scaling, const double& dcmip16_mu,
-                               const int& ftype, const int& theta_adv_form, const bool& prescribed_wind, const bool& moisture, const bool& disable_diagnostics,
-                               const bool& use_cpstar, const int& transport_alg, const bool& theta_hydrostatic_mode, const char** test_case,
+                               const int& ftype, const int& theta_adv_form, const int& prescribed_wind, const int& use_moisture, const int& disable_diagnostics,


Why changing the type of these boolean from bool to int?

bartgol · 2024-09-05T21:34:59Z

components/homme/src/theta-l_kokkos/prim_driver_mod.F90

    ! Initialize the C++ reference element structure (i.e., pseudo-spectral deriv matrix and ref element mass matrix)
    dvv = deriv1%dvv
    elem_mp = mp
    call init_reference_element_c(c_loc(dvv),c_loc(elem_mp))

    ! Fill the simulation params structures in C++
    test_name = TRIM(test_case) // C_NULL_CHAR
+


This seems overly complicated. We create an int from a bool to call the c function, and then inside the c function we convert the int back to a bool. Why not passing bool in the first place?

bartgol · 2024-09-05T21:36:20Z

components/homme/src/theta-l_kokkos/theta_f2c_mod.F90

@@ -138,11 +138,11 @@ end subroutine init_reference_element_c

  ! Create C++ functors
  subroutine init_functors_c (allocate_buffer) bind(c)
-  use iso_c_binding, only: c_bool


Also here, why the bool->int change?

bartgol · 2024-09-05T21:37:31Z

components/homme/cmake/machineFiles/chrysalis-bfb.cmake

@@ -17,6 +17,8 @@ ENDIF()
 SET (USE_MPIEXEC "srun" CACHE STRING "")
 SET (USE_MPI_OPTIONS "-K --cpu_bind=cores" CACHE STRING "")

+SET (CHRYSALIS_MACHINE TRUE CACHE BOOL "")


I don't see this var ever being used. Can we rm from these two mach files?

ambrad · 2024-09-06T00:19:43Z

Two points for consideration:

I agree with Luca that it's not clear the bool->int conversion to work around the system issue is, in its current form, as minimal as it can be. It might be worth thinking about whether the workaround can be implemented with substantially fewer modifications.
This PR has a lot of micro commits, 103. Ideally, these would be squashed to a few meaningful ones.

components/homme/src/theta-l_kokkos/cxx/ForcingFunctor.hpp

components/homme/src/prim_main.F90

oksanaguba · 2024-09-06T02:45:57Z

Thanks, Luca and Andrew. Luca used lazygit and made a new branch with a few commits out of this one. If you do not like int-bool changes, we can just wait till the issue resolves itself, and then redo this work. If possible, I would not want to spend more time cleaning that change (but i will check HV functor if i had an unnecessary change there).

oksanaguba · 2024-09-06T19:58:01Z

see #6594 .

Changes needed for spot/aurora: sycl kokkos spaces, temporary TP changes (TP init will be sorted out later), switch booleans to ints in F<-->xx interfaces. see #6569 for the original branch/history. [bfb]

Oksana Guba and others added 30 commits January 11, 2023 03:18

start

e2f5026

start

ee5e229

more

47b0f1c

more

1890820

more

fc9ba00

cache file

8bb583a

current state

6beec3b

hardcode TP pair

bf0df64

message in cmake

e7b3904

latest mods to cache files

5500266

switch linker

ade928d

new cache file

da17369

make changes for unit tests

7b5269b

exclude 1st round of timers

ca700bc

disable limiter in Euler functor

8a89aee

comment print message

64c0a4b

simple div test without mpi deps

2f1fd8b

modified cmake

4c78f55

Merge 'origin/oksanaguba/homme/simple-ut' into oksanaguba/homme/sycl

7efc71d

Conflicts: components/homme/CMakeLists.txt components/homme/src/share/gllfvremap_mod.F90 components/homme/test_execs/share_kokkos_ut/CMakeLists.txt

fix s1 ut for summit no io, no mpi build

4cd6047

resize ut

c4fa721

changes for external kokkos build

e8063de

disable printf

f6ceb2c

add simplified remap test

07c0aa9

add iterations to r1

caf2c2c

fix timing include for ut

1b44ac6

make div_wk, remap, s1 tests longer

b552b70

control nelems

1b3ac73

control nelems

a12e45c

temp fix for bool var in RemapStateProvider

f31735f

clean preqx ut cache

f1e724d

sort out gllfvremap mod

07d54c5

oksanaguba added 6 commits September 4, 2024 22:50

clean remapstateprovider from debug

0219b1c

remove unrelated comment

ad978ae

fix cmake

1bc79ab

Merge 'origin/master' into oksanaguba/homme/sycl

7e3ab70

Conflicts: components/homme/CMakeLists.txt components/homme/test_execs/share_kokkos_ut/CMakeLists.txt

remove old standalone kokkos building, use env Kokkos_ROOT instead

34a2973

remove remnants of kokkos build and simplified unit tests, formatting

0149b88

oksanaguba changed the title ~~[wip] sycl branch~~ SYCL in homme Sep 5, 2024

rljacob requested review from ambrad and bartgol September 5, 2024 20:14

rljacob assigned oksanaguba Sep 5, 2024

rljacob added HOMME Aurora labels Sep 5, 2024

rljacob requested a review from rebustamante September 5, 2024 20:38

oksanaguba removed the request for review from rebustamante September 5, 2024 20:38

bartgol reviewed Sep 5, 2024

View reviewed changes

ambrad reviewed Sep 6, 2024

View reviewed changes

components/homme/src/theta-l_kokkos/cxx/ForcingFunctor.hpp Show resolved Hide resolved

ambrad reviewed Sep 6, 2024

View reviewed changes

components/homme/src/prim_main.F90 Show resolved Hide resolved

oksanaguba closed this Sep 6, 2024

oksanaguba mentioned this pull request Sep 6, 2024

homme SYCL changes #6594

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SYCL in homme #6569

SYCL in homme #6569

oksanaguba commented Aug 28, 2024 •

edited

Loading

oksanaguba commented Aug 29, 2024

ambrad commented Aug 30, 2024

bartgol commented Sep 3, 2024 •

edited

Loading

oksanaguba commented Sep 5, 2024

bartgol left a comment

bartgol Sep 5, 2024

bartgol Sep 5, 2024

oksanaguba Sep 6, 2024

bartgol Sep 5, 2024

oksanaguba Sep 6, 2024

bartgol Sep 5, 2024

bartgol Sep 5, 2024

bartgol Sep 5, 2024

bartgol Sep 5, 2024

bartgol Sep 5, 2024

bartgol Sep 5, 2024

bartgol Sep 5, 2024

ambrad commented Sep 6, 2024

oksanaguba commented Sep 6, 2024

oksanaguba commented Sep 6, 2024

SYCL in homme #6569

SYCL in homme #6569

Conversation

oksanaguba commented Aug 28, 2024 • edited Loading

oksanaguba commented Aug 29, 2024

ambrad commented Aug 30, 2024

bartgol commented Sep 3, 2024 • edited Loading

oksanaguba commented Sep 5, 2024

bartgol 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

ambrad commented Sep 6, 2024

oksanaguba commented Sep 6, 2024

oksanaguba commented Sep 6, 2024

oksanaguba commented Aug 28, 2024 •

edited

Loading

bartgol commented Sep 3, 2024 •

edited

Loading