[SYCL] Ensure proper definition of spirv builtins for SYCL #1393
Conversation
Ensures proper definition of SPIRV global and workgroup builtin variables for both SYCL compilation with both NVPTX and SPIRV. Signed-off-by: Alexander Johnston <alexander@codeplay.com>
Signed-off-by: Alexander Johnston <alexander@codeplay.com>
|
This patch will allow us to avoid the changes to the spirv translator mentioned here #1166 Another commit will come soon with the changes removed from llvm-spirv |
Alexander-Johnston
changed the title
Removes the downstream changes from llvm-spirv, instead opting to implement the required spirv vars in the spirv_vars.hpp header. Due to the removal of the llvm-spirv changes, the libdevice library now requires access to spirv_vars.hpp to get all required spirv vars. Signed-off-by: Alexander Johnston <alexander@codeplay.com>
Alexander-Johnston
changed the title
| @@ -9,6 +9,10 @@ | |||
| #ifndef __LIBDEVICE_DEVICE_H__ | |||
| #define __LIBDEVICE_DEVICE_H__ | |||
|
|
|||
| // We need the following header provided by the sycl project to ensure | |||
| // definition of all spirv variables required by the wrapper libraries. | |||
| #include "spirv_vars.hpp" | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I see that #1384 removed all _spirv* declarations from libdevice/device_math.h, and I do not understand how it passed the testing. @bader, do you know?
I agree that devicelib must not depend on any headers from SYCL project. We are building devicelib in a fork without SYCL support, so I'd rather keep devicelib self-contained.
There was a problem hiding this comment.
I see that #1384 removed all _spirv* declarations from libdevice/device_math.h, and I do not understand how it passed the testing. @bader, do you know?
I agree that devicelib must not depend on any headers from SYCL project. We are building devicelib in a fork without SYCL support, so I'd rather keep devicelib self-contained.
With this change SPIR-V built-ins are "clang" built-ins i.e. they are recognized by clang w/o forward declarations as they are declared by clang itself.
There was a problem hiding this comment.
Thank you! I will have to use -fdeclare-spirv-builtins for non-SYCL builds of libdevice.
There was a problem hiding this comment.
This looks like a broken design.
libdeviceshould not includesycllibrary headers.
@vzakhari, recently moved this library out of thesyclproject to make them independent (i.e. we should be able to one w/o the other).Doesn't #1384 help to have all needed definitions?
This is required because the _devicelib_assert_fail calls in glibc_wrapper.cpp and msvc_wrapper.cpp both require the global and local invocation IDs.
__devicelib_assert_fail(
expr, file, line, func, __spirv_GlobalInvocationId_x(),
__spirv_GlobalInvocationId_y(), __spirv_GlobalInvocationId_z(),
__spirv_LocalInvocationId_x(), __spirv_LocalInvocationId_y(),
__spirv_LocalInvocationId_z());
The upstream llvm-spirv translator doesn't implement these, so we implemented them in a somewhat broken way in the llvm-spirv translator, see #1166. This patch is triyng to avoid upstreaming the broken changes to the translator by implementing the invocation ID functions (among some others) for non NVPTX inside spirv_vars.hpp, which is why the devicelib has a requirement on the SYCL header.
If you'd prefer, an alternative to the header dependency would be to have another header in the devicelib which implements only the required invocation ID functions.
There was a problem hiding this comment.
Yes, please make the implementation in a devicelib header. Not that I like it, but I want devicelib to be self-contained, and do not see a better way now.
There was a problem hiding this comment.
Thank you, @Alexander-Johnston! LGTM for libdevice.
Rather than have a dependency on SYCL headers in libdevice, implement the required spirv builtins in the libdevice project. Signed-off-by: Alexander Johnston <alexander@codeplay.com>
Alexander-Johnston
force-pushed
the
Alex/spirv-builtin-definitions
branch
from
d90cec0 to
41ea7ab
Compare
Accidentally removed the inclusion of spirv_vars.hpp in device.h in 41ea7ab. Reinclude it here. Signed-off-by: Alexander Johnston <alexander@codeplay.com>
| @@ -6,6 +6,10 @@ | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| // We need the following header to ensure the definition of all spirv variables | |||
| // required by the wrapper libraries. | |||
| #include "spirv_vars.hpp" | |||
There was a problem hiding this comment.
I would move this include below the guard.
Signed-off-by: Alexander Johnston <alexander@codeplay.com>
Alexander-Johnston
requested review from
asavonic and
romanovvlad
as code owners
|
@AlexeySachkov, @AlexeySotkin, @asavonic, and/or @vzakhari, your approval is required to merge this PR. |
|
@AlexeySachkov, @AlexeySotkin, ping. |
…duler_docs * origin/sycl: [SYCL][PI][CUDA] Implements get_native interoperability (intel#1332) [SYCL] Fix check-sycl test suite on systems w/o OpenCL (intel#1503) [SYCL][Doc] Update ExtendedAtomics documentation (intel#1487) [SYCL][CUDA] Expose context extended deleters on PI API (intel#1483) [SYCL][NFC] Remove a dropped environment variable from a test (intel#1506) [SYCL] Add opencl-aot to sycl-toolchain target (intel#1504) [SYCL] Allow to run deploy LIT tests from particular directory [SYCL][CUDA] Fix LIT testing with CUDA devices (intel#1300) [SYCL] Remove operator name keywords (intel#1501) [Driver][SYCL] Consider .lo files as static archives (intel#1500) [SYCL-PTX] Update the compiler design to describe the CUDA target (intel#1408) [SYCL] Fix library build on Windows (intel#1499) [SYCL][NFC] Refactor lit.cfg.py (intel#1452) [SYCL] Fixed sub-buffer memory allocation update (intel#1486) [SYCL] Ensure proper definition of spirv builtins for SYCL (intel#1393) [SYCL][CUDA] LIT XFAIL/UNSUPPORTED (intel#1303) [SYCL][Doc] Function-type kernel attribute extension (intel#1494)
Currently the spirv_vars header only ensures proper definition of the required spirv builtin vars for NVPTX. This patch ensures proper definitions are also available to other compilation targets by implementing the functions in the header in terms of the spirv builtin when not compiling for NVPTX.
Signed-off-by: Alexander Johnston alexander@codeplay.com