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

Update SYCL support discovery to check for -fsycl flag. #450

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Update SYCL support discovery to check for -fsycl flag. #450

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions cmake/mkl/MKLConfig.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,12 +208,28 @@ if(CMAKE_Fortran_COMPILER)
endif()

# Determine Compiler Family
if(CXX_COMPILER_NAME STREQUAL "dpcpp" OR CXX_COMPILER_NAME STREQUAL "dpcpp.exe"
OR CXX_COMPILER_NAME STREQUAL "icpx" OR CXX_COMPILER_NAME STREQUAL "icx.exe")
set(SYCL_COMPILER ON)

include(CMakePackageConfigHelpers)
include(CheckCXXCompilerFlag)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you put this block in a condition please?

if("CXX" IN_LIST CURR_LANGS)
  include(CheckCXXCompilerFlag)
  include(CheckIncludeFileCXX)

  check_cxx_compiler_flag("-fsycl" _fsycl_option)
  ...
      if (_sycl_header OR _sycl_header_old)
      set(SYCL_COMPILER ON)
    endif()
  endif()
endif()

Although only CXX is used in oneMKL interfaces, oneMKL also needs to support C and Fortran. There is a possibility that users would use MKLConfig.cmake here with oneMKL.

include(CheckIncludeFileCXX)
include(GNUInstallDirs)
Comment on lines +212 to +215
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
include(CMakePackageConfigHelpers)
include(CheckCXXCompilerFlag)
include(CheckIncludeFileCXX)
include(GNUInstallDirs)
include(CheckCXXCompilerFlag)
include(CheckIncludeFileCXX)

I think we can delete the first line include(CMakePackageConfigHelpers) and the fourth line include(GNUInstallDirs). CheckCXXCompilerFlag is needed for check_cxx_compiler_flag(), and CheckIncludeFileCXX is needed for CHECK_INCLUDE_FILE_CXX().


# Check SYCL support by the compiler
check_cxx_compiler_flag("-fsycl" _fsycl_option)
if (_fsycl_option)
CHECK_INCLUDE_FILE_CXX("sycl/sycl.hpp" _sycl_header "-fsycl")
if (NOT _sycl_header)
CHECK_INCLUDE_FILE_CXX("CL/sycl.hpp" _sycl_header_old "-fsycl")
Comment on lines +218 to +222
Copy link
Contributor

@dnhsieh-intel dnhsieh-intel Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

SYCL_SYCL_HPP and CL_SYCL_HPP might be more expressive than _sycl_header and _sycl_header_old. It would be great if you also use COMPILER_FSYCL_SUPPORT instead of _fsycl_option. Please note that the convention in MKLConfig.cmake is to use all caps for variable names.

In addition, please consider removing the space after if to match the style in the file:

if(<condition>)

endif()
if (_sycl_header OR _sycl_header_old)
set(SYCL_COMPILER ON)
endif()
endif()
if(C_COMPILER_NAME MATCHES "^clang" OR CXX_COMPILER_NAME MATCHES "^clang")
set(CLANG_COMPILER ON)

if(NOT DEFINED SYCL_COMPILER OR SYCL_COMPILER MATCHES OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach could not work correctly for case when MKLConfig is used for building non-SYCL app with clang compiler that supports SYCL, but I believe this fix is enough for oneMKL project build. We can update this part later with more general fix from Intel oneMKL

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean

Suggested change
if(NOT DEFINED SYCL_COMPILER OR SYCL_COMPILER MATCHES OFF)
if(NOT SYCL_COMPILER)

if(C_COMPILER_NAME MATCHES "^clang" OR CXX_COMPILER_NAME MATCHES "^clang")
set(CLANG_COMPILER ON)
endif()
endif()
if(CMAKE_C_COMPILER_ID STREQUAL "PGI" OR CMAKE_CXX_COMPILER_ID STREQUAL "PGI" OR CMAKE_Fortran_COMPILER_ID STREQUAL "PGI"
OR CMAKE_C_COMPILER_ID STREQUAL "NVHPC" OR CMAKE_CXX_COMPILER_ID STREQUAL "NVHPC"
Expand Down