-
Notifications
You must be signed in to change notification settings - Fork 798
[SYCL] kernel_compiler remove GCC < 8 workarounds. #16550
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
[SYCL] kernel_compiler remove GCC < 8 workarounds. #16550
Conversation
…ve to forego them.
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.
LGTM.
#include <experimental/filesystem> | ||
namespace fs = std::experimental::filesystem; | ||
#else | ||
#error "kernel_compiler sycl requires C++ filesystem support" |
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.
#error "kernel_compiler sycl requires C++ filesystem support" | |
#error "kernel_compiler requires C++ filesystem support" |
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.
The kernel_compiler requires C++ filesystem support for compiling SYCL C++ code. But it can compile OpenCL C without it.
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.
Sorry, I don't understand the meaning of sycl
in this context. Is this the part of the file name or the name of the standard?
If this text refers to the name of the standard it must use capital letters.
Where is kernel_compiler
term defined? Is this some abbreviation of the sycl_ext_oneapi_kernel_compiler
extension name?
// and much of the cross-platform file handling code depends upon it. | ||
// Given that this extension is experimental and that the file | ||
// handling aspects are most likely temporary, it makes sense to | ||
// simply not support GCC<8. |
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.
According to this comment DPC++ doesn't support GCC 7 and older.
[SYCL] kernel_compiler remove GCC < 8 workarounds.
I would change the PR description to say that this patch adds support for GCC 7 rather than just removes the GCC< 8 workarounds. It's hard to say which GCC version is guaranteed to work, but I think GCC 7 is the first version supporting most of C++17 features. C++17 support is the main requirement for SYCL host compiler.
We can use experimental filesystem when compiling SYCL with GCC 7, which means we can remove the #ifdef hacks from the kernel_compiler.
We can use experimental filesystem when compiling SYCL with GCC 7, which means we can remove the #ifdef hacks from the kernel_compiler.