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.

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

Fix CTests in SYCL runs #153

Merged
merged 5 commits into from
Jan 29, 2025

Conversation

amametjanov
Copy link
Member

@amametjanov amametjanov commented Oct 29, 2024

Fix CTests in SYCL runs:

  • Move -Xsycl-target-backend to link flags
  • Fix SYCL and PBS flags

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)

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.

components/omega/CMakeLists.txt Show resolved Hide resolved
components/omega/src/base/IO.h Outdated Show resolved Hide resolved
components/omega/test/CMakeLists.txt Show resolved Hide resolved
components/omega/test/CMakeLists.txt Show resolved Hide resolved
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."

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding generic SYCL flags from upstream-E3SM here. Omega-specific flags are added with OMEGA_SYCL_EXE_LINKER_FLAGS .

Choose a reason for hiding this comment

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

I guess that SYCL_FLAGS is defined somewhere in E3SM (or CIME). Do you allow users to add -DSYCL_FLAGS in the Omega CMake command line for standalone build? If allowed, I think we may still want to use OMEGA_SYCL_FLAGS in addition to SYCL_FLAGS for naming consistency similar to:

if(SYCL_FLAGS)
   set(OMEGA_SYCL_FLAGS "${OMEGA_SYCL_FLAGS} ${SYCL_FLAGS}")
endif()

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, SYCL_FLAGS is coming from E3SM's cime_config/machines/cmake_macros/oneapi-ifxgpu_$MACHINE.cmake. I added a commit to keep OMEGA_SYCL_FLAGS .

components/omega/src/CMakeLists.txt Show resolved Hide resolved
@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.

Copy link
Member Author

@amametjanov amametjanov left a comment

Choose a reason for hiding this comment

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

@grnydawn please see below: I tried to address all of the comments, thanks.

components/omega/CMakeLists.txt Show resolved Hide resolved
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
Member Author

Choose a reason for hiding this comment

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

Adding generic SYCL flags from upstream-E3SM here. Omega-specific flags are added with OMEGA_SYCL_EXE_LINKER_FLAGS .

components/omega/src/CMakeLists.txt Show resolved Hide resolved
components/omega/test/CMakeLists.txt Show resolved Hide resolved
components/omega/test/CMakeLists.txt Show resolved Hide resolved
@amametjanov amametjanov marked this pull request as ready for review January 28, 2025 17:10
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.

Approved. All tests on Frontier and Chrysalis have passed. We can revisit the SYCL_FLAGS discussion if needed in the future. Thanks!

@grnydawn grnydawn assigned grnydawn and philipwjones and unassigned grnydawn Jan 28, 2025
@philipwjones philipwjones merged commit cc4eb05 into E3SM-Project:develop Jan 29, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants