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

Make SYCL backend detection more portable #1881

Merged
merged 30 commits into from
Nov 15, 2024

Conversation

dmitriy-sobolev
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev commented Oct 4, 2024

SYCL specification allows SYCL to be implemented purely as a library: https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#_library_only_implementation. It means that inclusion of sycl/sycl.hpp is a portable way to get SYCL_LANGUAGE_VERSION, in contrast to expecting it to be defined by the compiler itself. Additionally, AdaptiveCPP defines this macro in sycl/sycl.hpp.

Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

I have a memory of us running into issues with some compilers providing sycl.hpp, but not actually providing a working SYCL implementation when we were adding this to CMake detection of the availability of SYCL. Wouldn't this have the potential for some problems in that case?

My memory is that this was with clang++, but I may be mistaken. Let me see if I can pull up that discussion.

@danhoeflinger
Copy link
Contributor

I have a memory of us running into issues with some compilers providing sycl.hpp, but not actually providing a working SYCL implementation when we were adding this to CMake detection of the availability of SYCL. Wouldn't this have the potential for some problems in that case?

My memory is that this was with clang++, but I may be mistaken. Let me see if I can pull up that discussion.

It turns out its the opposite issue in reality #959 (comment)
fsycl option was available but headers not available.

@akukanov
Copy link
Contributor

akukanov commented Oct 7, 2024

Let's do some analysis first.

__ONEDPL_SYCL_AVAILABLE expectedly says whether it is possible to use SYCL code. It is then used to
a) check SYCL availability when it is requested via ONEDPL_USE_DPCPP_BACKEND
b) enable SYCL backend implicitly when not requested

There are two SYCL availability tests we use:

  1. check if <sycl/sycl.hpp> is accessible
  2. check if SYCL_LANGUAGE_VERSION is defined.

According to the SYCL standard, eventually both checks need to pass.
(1) is a mandatory condition prior for any use of SYCL, while (2) might depend on the implementation, as Dmitriy points out.
For some compilers, including DPC++, (2) is independently enabled by a compiler option.
For other compilers or library only implementations, (2) might never be true until the header is included. However even after the header is included, (2) is not always true I think, and so it still needs to be checked.

So the simplest generic SYCL availability test would be:

  1. check if <sycl/sycl.hpp> is accessible
    1.5) include <sycl/sycl.hpp>
  2. check if SYCL_LANGUAGE_VERSION is defined

However, we do not want to include the SYCL header until it's needed. Also, if the header code depends on compiler support,
including it without prior checking (2) might in theory cause errors. So an intermediate state "SYCL is possibly available" - which essentially means we observe the header but not the language macro - seems useful.

Also note that the use cases (a) and (b) would treat "possibly available" differently:

  • if the use of SYCL is explicitly requested, i.e. ONEDPL_USE_DPCPP_BACKEND is set to a non-zero value,
    we may assume that the environment is ready for that, and including the header seems reasonable.
  • if there was no direct request for SYCL, we need to be cautious and should not include the header
    unless we are sure it will not cause errors.

The general procedure could be:

  1. check if <sycl/sycl.hpp> is accessible;
    • if no, SYCL is definitely not available.
  2. check if SYCL_LANGUAGE_VERSION is defined;
    • if yes, SYCL is definitely available.
    • if no, but we know it is required (as for Intel Compiler), SYCL is definitely not available.
    • otherwise, SYCL is possibly available
  3. Case (a): if the use of SYCL is requested but SYCL is definitely not available, issue an error.
  4. If the use of SYCL is requested and SYCL is [possibly] available (i.e. not definitely unavailable), enable the SYCL backend.
  5. Case (b): If the use of SYCL is not requested but we are sure including the header is safe, enable the SYCL backend.
    • Currently, we are only sure when SYCL is definitely available, but we can expand it later for "known good" environments.

With this procedure, the SYCL backend might be enabled without requiring SYCL_LANGUAGE_VERSION a priori,
and therefore the backend should check SYCL_LANGUAGE_VERSION after including the header:

  1. include <sycl/sycl.hpp>
  2. if SYCL_LANGUAGE_VERSION is still not defined:
    • complete the case (a): if SYCL was requested, issue an error.
    • disable the SYCL backend and skip the rest of it.
    • alternatively, if disabling the SYCL backend from inside does not seem feasible, just issue an error.

@dmitriy-sobolev
Copy link
Contributor Author

dmitriy-sobolev commented Oct 15, 2024

Thank you for such a detailed analysis, Alexey. I've implemented this suggestion.

@dmitriy-sobolev dmitriy-sobolev force-pushed the dev/dmitriy-sobolev/sycl-version branch from f1fbb2d to e8cedef Compare October 15, 2024 15:32
Copy link
Contributor

@akukanov akukanov left a comment

Choose a reason for hiding this comment

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

I have some questions and suggestions.

Also, there is no check in the backend that the language version macro is properly defined after the header is included.

CMakeLists.txt Outdated Show resolved Hide resolved
include/oneapi/dpl/pstl/onedpl_config.h Outdated Show resolved Hide resolved
include/oneapi/dpl/pstl/onedpl_config.h Outdated Show resolved Hide resolved
test/support/test_config.h Outdated Show resolved Hide resolved
@dmitriy-sobolev dmitriy-sobolev marked this pull request as draft October 18, 2024 19:31
@dmitriy-sobolev dmitriy-sobolev changed the title Do not expect a genetic compiler to define SYCL_LANGUAGE_VERSION Do not expect a compiler to define SYCL_LANGUAGE_VERSION Oct 21, 2024
@dmitriy-sobolev dmitriy-sobolev force-pushed the dev/dmitriy-sobolev/sycl-version branch from 99d3614 to 6e6e985 Compare October 21, 2024 17:31
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Move hetero backend configuration into a separate file

Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
@dmitriy-sobolev dmitriy-sobolev force-pushed the dev/dmitriy-sobolev/sycl-version branch 2 times, most recently from b0c078b to 85d7c67 Compare October 22, 2024 13:35
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
@dmitriy-sobolev dmitriy-sobolev marked this pull request as ready for review October 22, 2024 18:34
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
@dmitriy-sobolev dmitriy-sobolev changed the title Do not expect a compiler to define SYCL_LANGUAGE_VERSION Make SYCL backend detection more portable Oct 24, 2024
include/oneapi/dpl/pstl/onedpl_config.h Outdated Show resolved Hide resolved
include/oneapi/dpl/pstl/onedpl_config.h Outdated Show resolved Hide resolved
include/oneapi/dpl/pstl/onedpl_config.h Outdated Show resolved Hide resolved
include/oneapi/dpl/pstl/onedpl_config.h Outdated Show resolved Hide resolved
include/oneapi/dpl/pstl/onedpl_config.h Show resolved Hide resolved
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
akukanov
akukanov previously approved these changes Nov 13, 2024
Copy link
Contributor

@akukanov akukanov left a comment

Choose a reason for hiding this comment

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

It looks good to me.

danhoeflinger
danhoeflinger previously approved these changes Nov 14, 2024
Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

LGTM, these are all good changes. If you want to take the optional nitpicks, I will approve after

Signed-off-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

LGTM, the clang format suggestion is bad and should be ignored.

@dmitriy-sobolev dmitriy-sobolev merged commit 658621d into main Nov 15, 2024
21 of 22 checks passed
@dmitriy-sobolev dmitriy-sobolev deleted the dev/dmitriy-sobolev/sycl-version branch November 15, 2024 14:34
@akukanov akukanov modified the milestone: 2022.8.0 Dec 17, 2024
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