Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix CTests in SYCL runs #153

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft

Fix CTests in SYCL runs #153

wants to merge 5 commits into from

Conversation

amametjanov
Copy link
Member

@amametjanov amametjanov commented Oct 29, 2024 •
edited
Loading

Fix CTests in SYCL runs:

  • Move -Xsycl-target-backend to link flags
  • Fix SYCL and PBS flags
  • Default to PIO_IOTYPE_PNETCDF to work around HDF5 in SYCL runs

Checklist

  • Testing
    • A comment in the PR documents testing used to verify the changes including any tests that are added/modified/impacted.
    • Unit tests have passed. Please provide a relevant CDash build entry for verification.

@amametjanov
Copy link
Member Author

Testing:

  • most of the tests are passing for both oneapi-ifx on CPUs and oneapi-ifxgpu on GPUs.
  • need to fix IO_TEST: NetCDF _FillValue type mismatch error
96% tests passed, 1 tests failed out of 28 

Total Test time (real) = 179.80 sec

The following tests FAILED:
         15 - IO_TEST (Failed)

grnydawn
grnydawn requested changes Nov 1, 2024
View reviewed changes
Copy link

@grnydawn grnydawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comments below.

cime_config/machines/cmake_macros/oneapi-ifxgpu_aurora.cmake Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we’d prefer not to change the E3SM machine file in an Omega PR. This change would be better suited for an E3SM PR, and we can downstream the change to Omega afterward.

cime_config/machines/cmake_macros/oneapi-ifxgpu_sunspot.cmake Outdated
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the above comment, we can downstream the change from E3SM to Omega.

components/omega/CMakeLists.txt
@@ -44,7 +44,7 @@ if (NOT DEFINED PROJECT_NAME)

set(CMAKE_CXX_STANDARD 17) # used in E3SM
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(LINKER_LANGUAGE C) # needed to support Cray C compiler wrapper
set(LINKER_LANGUAGE CXX) # needed to support Cray C compiler wrapper
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Frontier, there was an issue with linking to the CXX compiler because mipcxx was set to CXX instead of the CC Cray compiler wrapper. During the linking phase, the Omega compilation failed. Time have passed since then, and a quick build with this change now seems to pass the linking stage, but please ensure that this change works correctly with all compilers on Frontier.

components/omega/src/base/IO.h
@@ -68,7 +68,7 @@ enum FileFmt {
FmtHDF5 = PIO_IOTYPE_HDF5, ///< native HDF5 format
FmtADIOS = PIO_IOTYPE_ADIOS, ///< ADIOS format
FmtUnknown = -1, ///< Unknown or undefined
FmtDefault = PIO_IOTYPE_NETCDF4C, ///< NetCDF4 is default
FmtDefault = PIO_IOTYPE_PNETCDF, ///< work around HDF5 in SYCL runs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change, setting the default I/O type to PNETCDF, work on all machines, including Frontier, Perlmutter, and Chrysalis?

components/omega/test/CMakeLists.txt
@@ -18,14 +18,22 @@ function(add_omega_test test_name exe_name source_files mpi_args)
OmegaLibFlags
)

set_target_properties(${exe_name} PROPERTIES LINKER_LANGUAGE C)
set_target_properties(${exe_name} PROPERTIES LINKER_LANGUAGE CXX)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the above comment about using C linker instead of CXX.

components/omega/test/CMakeLists.txt
if("${OMEGA_ARCH}" STREQUAL "SYCL")
add_test(
NAME ${test_name}
COMMAND ${OMEGA_MPI_EXEC} ${mpi_args} ${OMEGA_MPI_ARGS} ./${exe_name}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious why the order of MPI arguments matters.

components/omega/OmegaBuild.cmake
if(OMEGA_SYCL_FLAGS)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OMEGA_SYCL_FLAGS}")
if(SYCL_FLAGS)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${SYCL_FLAGS}")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to add the "OMEGA_" prefix to all Omega-related CMake variables and prefer using "OMEGA_SYCL_FLAGS" instead of "SYCL_FLAGS."

components/omega/src/CMakeLists.txt
@@ -74,6 +74,6 @@ if(OMEGA_BUILD_EXECUTABLE)
OmegaLibFlags
)

set_target_properties(${OMEGA_EXE_NAME} PROPERTIES LINKER_LANGUAGE C)
set_target_properties(${OMEGA_EXE_NAME} PROPERTIES LINKER_LANGUAGE CXX)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the above comment about using C linker instead of CXX.

@amametjanov
Copy link
Member Author

This PR depends on E3SM-Project#6715 : E3SM tests with cmake changes in this PR appear to be okay. I'll update as soon as this PR is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grnydawn grnydawn grnydawn requested changes

Assignees
No one assigned
Labels
Omega
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@amametjanov @grnydawn