-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix CTests in SYCL runs #153
Conversation
Testing:
|
There was a problem hiding this 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/OmegaBuild.cmake
Outdated
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}") |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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
.
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. |
de17ea3
to
be5c90c
Compare
e4f899d
to
4f666d8
Compare
There was a problem hiding this 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/OmegaBuild.cmake
Outdated
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}") |
There was a problem hiding this comment.
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
.
There was a problem hiding this 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!
Fix CTests in SYCL runs:
-Xsycl-target-backend
to link flagsChecklist