-
Notifications
You must be signed in to change notification settings - Fork 745
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][NATIVECPU][LIBCLC] Use libclc for SYCL Native CPU #10970
Conversation
This reverts commit bcb4387.
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.
+1 on common code
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.
FE changes LGTM
Hello @intel/dpcpp-devops-reviewers @intel/llvm-reviewers-runtime @intel/llvm-reviewers-cuda @intel/dpcpp-clang-driver-reviewers, can I get some feedback on this PR? Thank you :) |
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.
Overall it looks good, but why is it we need to add all those empty .cl files? Does LIBCLC assume their existence for all targets?
libclc/CMakeLists.txt
Outdated
@@ -181,6 +181,8 @@ set( nvptx--nvidiacl_devices none ) | |||
set( nvptx64--nvidiacl_devices none ) | |||
set( spirv-mesa3d-_devices none ) | |||
set( spirv64-mesa3d-_devices none ) | |||
# todo: does this need to be set for each possible triple? |
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.
Please follow the comment formatting used throughout the rest of the file. I.e.
# todo: does this need to be set for each possible triple? | |
# TODO: Does this need to be set for each possible triple? |
Yes, my understanding is that the libclc CMake config falls back to the |
I agree, this is probably the least intrusive solution to the imposed problem. Thanks! |
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.
FE changes OK for me!
Hello @intel/llvm-gatekeepers I think this is ready to be merged, thanks :) |
This PR allows linking to libclc when compiling for SYCL Native CPU. Currently only the
x86_64-unknown-linux-gnu
target triple is supported, additional target triples (and possibly a more versatile way of setting them) will come with follow up PRs.Some useful information for reviewing:
AddrSpaceMap
(set inTargetInfo.cpp
) because the mangled names emitted by the device compiler need to match with the names provided bylibclc
. The AddressSpaceMap is taken from thePTX
Target.Driver
are needed to find and link tolibclc
.libclc/ptx-nvidiacl/libspirv/atomic/loadstore_helpers.ll
has been split into 4 modules, one for each memory ordering constraint. Copies of these modules have been added ingeneric
(because some functions ingeneric/libspirv/atomic
needed them), and the module split allows to specialize the file for targets that may not support some orderings. Currently only a couple of function foracquire
andseq_cst
have been implemented forgeneric
, but the others will be implemented in a follow up PR.libclc
forx86_64-unknown-linux
. This has been done because some math builtins ingeneric
have been defined asWhile this approach conveniently allows to call directly LLVM intrinsics, it does seem to play well with the ABI for
x86_64-unknown-linux
, since it leads to this IR:Which is invalid because
lvm.ctpop.v8i8
expect a vector ofi8
and not adouble
, leading to failing asserts in the compiler that prevented from buildinglibclc
.As a temporary work around we have added empty files that override the files in
generic
when building forx86_64-unknown-linux
, allowing to complete the build, even though the corresponding builtins will be missing from the library. We are working on a proper solution for this.