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

Remove support of old compilers from oneDPL code #1678

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

SergeyKopienko
Copy link
Contributor

@SergeyKopienko SergeyKopienko commented Jul 8, 2024

As far as our supported compiler is 2022.2.0 (int's started from Intel oneAPI 2022.3), we able to remove the code for older compilers.

For 2022.2.0, the _ONEDPL_LIBSYCL_VERSION is equal to 50700.

This mean that all comparison of _ONEDPL_LIBSYCL_VERSION with 50700 or some less values not required anymore.
So in this PR we removes the next parts of code:

  • _ONEDPL_LIBSYCL_VERSION >= 50200
  • _ONEDPL_LIBSYCL_VERSION >= 50300
  • _ONEDPL_LIBSYCL_VERSION >= 50400
  • _ONEDPL_LIBSYCL_VERSION >= 50700

Also we add minimal compiler version check here :

// Combine SYCL runtime library version
#if defined(__LIBSYCL_MAJOR_VERSION) && defined(__LIBSYCL_MINOR_VERSION) && defined(__LIBSYCL_PATCH_VERSION)
#    define _ONEDPL_LIBSYCL_VERSION                                                                                    \
       (__LIBSYCL_MAJOR_VERSION * 10000 + __LIBSYCL_MINOR_VERSION * 100 + __LIBSYCL_PATCH_VERSION)
#    if _ONEDPL_LIBSYCL_VERSION < 57000
#        error "oneDPL requires Intel(R) oneAPI DPC++/C++ Compiler 2022.2.0 or newer"
#    endif
#else
#    define _ONEDPL_LIBSYCL_VERSION 0
#endif

For check macros state please take a look at https://godbolt.org/z/ne1z7n36b :

  • for icx 2022.2.0 the state of macros is:
__LIBSYCL_MAJOR_VERSION = 5
__LIBSYCL_MINOR_VERSION = 7
__LIBSYCL_PATCH_VERSION = 0

}

template <typename _Item>
constexpr void
__group_barrier(_Item __item)
{
#if 0 //_ONEDPL_LIBSYCL_VERSION >= 50300
#if 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MikeDvorskiy what we should to do with this #if 0 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

 //TODO: usage of sycl::group_barrier: probably, we have to revise SYCL parallel patterns which use a group_barrier.
// 1) sycl::group_barrier() implementation is not ready
// 2) sycl::group_barrier and sycl::item::group_barrier are not quite equivalent

Copy link
Contributor

@MikeDvorskiy MikeDvorskiy Jul 8, 2024

Choose a reason for hiding this comment

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

sycl::group_barrier() did not work at that moment.
I think you should double check the semantic and if it equivalent, figure out from which SYCL library version sycl::group_barrier() is ready and work properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MikeDvorskiy, could you please explain, what exactly did not work and in which cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue created: #1679
let's fix it in the separate PR later.

@SergeyKopienko SergeyKopienko marked this pull request as ready for review July 8, 2024 10:58
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/remove_old_compilers_from_code branch from 6a8d0af to 71dcd1a Compare July 8, 2024 11:28
@SergeyKopienko SergeyKopienko added this to the 2022.7.0 milestone Jul 8, 2024
Comment on lines +35 to +37
# if _ONEDPL_LIBSYCL_VERSION < 57000
# error "oneDPL requires Intel(R) oneAPI DPC++/C++ Compiler 2022.2.0 or newer"
# endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Just generally... does it make sense to "break" all compiles outside of our supported compilers, or merely "warn" that they may not work properly?

The way I look at the support compilers is that these are the compilers we must support and are committed to fixing issue with, rather than these are the only compilers we allow to use the library.

From that perspective, I would argue we should only make changes to explicitly remove support if it benefits us in some specific way (even if that specific way is merely that it makes the code significantly easier to maintain).

I haven't yet investigated all the individual changes of the PR, but it seems to me we shouldn't go out of our way to stop people from using the code with an out of date compiler. If we do decide to have such an error, I would recommend we also add a deprecation-style warning for compilers which are about to leave our window of support in the next 6-12 months.

@SergeyKopienko SergeyKopienko marked this pull request as draft July 8, 2024 13:45
@SergeyKopienko SergeyKopienko removed this from the 2022.7.0 milestone Jul 12, 2024
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/remove_old_compilers_from_code branch 2 times, most recently from a79ae82 to 8c802b6 Compare September 12, 2024 08:52
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/remove_old_compilers_from_code branch 2 times, most recently from da7c8ac to aaaa5e3 Compare September 18, 2024 09:16
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/remove_old_compilers_from_code branch from aaaa5e3 to 661933a Compare September 24, 2024 08:20
…er version check to _ONEDPL_LIBSYCL_VERSION >= 57000

Signed-off-by: Sergey Kopienko <[email protected]>
Signed-off-by: Sergey Kopienko <[email protected]>
…remove _USE_KERNEL_DEVICE_SPECIFIC_API due _ONEDPL_LIBSYCL_VERSION >= 50700 is always true

Signed-off-by: Sergey Kopienko <[email protected]>
@SergeyKopienko SergeyKopienko force-pushed the dev/skopienko/remove_old_compilers_from_code branch from 661933a to 35dd1fb Compare September 25, 2024 08:27
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.

3 participants