-
Notifications
You must be signed in to change notification settings - Fork 113
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
base: main
Are you sure you want to change the base?
Remove support of old compilers from oneDPL code #1678
Conversation
} | ||
|
||
template <typename _Item> | ||
constexpr void | ||
__group_barrier(_Item __item) | ||
{ | ||
#if 0 //_ONEDPL_LIBSYCL_VERSION >= 50300 | ||
#if 0 |
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.
@MikeDvorskiy what we should to do with this #if 0
?
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.
//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
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.
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.
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.
@MikeDvorskiy, could you please explain, what exactly did not work and in which cases?
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.
Issue created: #1679
let's fix it in the separate PR later.
6a8d0af
to
71dcd1a
Compare
# if _ONEDPL_LIBSYCL_VERSION < 57000 | ||
# error "oneDPL requires Intel(R) oneAPI DPC++/C++ Compiler 2022.2.0 or newer" | ||
# 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.
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.
a79ae82
to
8c802b6
Compare
da7c8ac
to
aaaa5e3
Compare
aaaa5e3
to
661933a
Compare
Signed-off-by: Sergey Kopienko <[email protected]>
Signed-off-by: Sergey Kopienko <[email protected]>
…>= 50300) Signed-off-by: Sergey Kopienko <[email protected]>
…ON >= 50300) Signed-off-by: Sergey Kopienko <[email protected]>
…ERSION >= 50300) Signed-off-by: Sergey Kopienko <[email protected]>
…= 50500) Signed-off-by: Sergey Kopienko <[email protected]>
Signed-off-by: Sergey Kopienko <[email protected]>
…SYCL_VERSION >= 50300 in __group_barrier(_Item __item) Signed-off-by: Sergey Kopienko <[email protected]>
…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]>
661933a
to
35dd1fb
Compare
As far as our supported compiler is
2022.2.0
(int's started from Intel oneAPI2022.3
), we able to remove the code for older compilers.For
2022.2.0
, the_ONEDPL_LIBSYCL_VERSION
is equal to50700
.This mean that all comparison of
_ONEDPL_LIBSYCL_VERSION
with50700
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 :
For check macros state please take a look at https://godbolt.org/z/ne1z7n36b :
icx 2022.2.0
the state of macros is: