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

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

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

Conversation

BenBrock
Copy link

@BenBrock BenBrock commented Feb 22, 2024

Description

oneMKL's CMake currently checks CXX_COMPILER_NAME and uses that to determine whether SYCL is supported. This prevents SYCL support from being detected if the user compiles with their own build of intel/llvm (see issue #449).

This is a first stab at modifying MKLConfig.cmake to check for -fsycl support directly instead of inferring from the compiler name. It is based on how oneDPL checks for SYCL support, which works for both release builds of IntelLLVM and custom builds of intel/llvm.

I have also modified the CMake to avoid setting CLANG_COMPILER if SYCL_COMPILER is set, as this is the behavior when using IntelLLVM. I assume open-source DPC++ builds should behave similarly.

Fixes #449

Checklist

All Submissions

  • Do all unit tests pass locally? Attach a log.
  • Have you formatted the code using clang-format?

New interfaces

  • Have you provided motivation for adding a new feature as part of RFC and
    it was accepted? # (RFC)
  • What version of oneAPI spec the interface is targeted?
  • Complete New features checklist

New features

  • Have you provided motivation for adding a new feature?
  • Have you added relevant tests?

Bug fixes

  • Have you added relevant regression tests?
  • Have you included information on how to reproduce the issue (either in a
    GitHub issue or in this PR)?

Copy link
Contributor

@mkrainiuk mkrainiuk left a comment

Choose a reason for hiding this comment

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

Thank you for the fix! I have one minor comment but overall looks good to me.

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

@dnhsieh-intel dnhsieh-intel left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I have some suggestions.

Comment on lines +212 to +215
include(CMakePackageConfigHelpers)
include(CheckCXXCompilerFlag)
include(CheckIncludeFileCXX)
include(GNUInstallDirs)
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().

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.

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.

Did you mean

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

Comment on lines +218 to +222
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")
Copy link
Contributor

@dnhsieh-intel dnhsieh-intel Mar 20, 2024

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>)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

oneMKL's CMake Does Not Detect SYCL Support when Compiled with intel/llvm
3 participants