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

[SYCL] Move SYCL headers from standard clang location #1308

Merged
merged 2 commits into from
Mar 15, 2020

Conversation

vladimirlaz
Copy link
Contributor

@vladimirlaz vladimirlaz commented Mar 13, 2020

SYCL headers were moved from default clang include directory
(lib/clang/11.0.0/include) to (lib/clang/11.0.0/include/sycl) to
support mixed compilers (e.g. gcc for host code and clang for
device code) during build of SYCL application.

Clang driver adds new directory to system include search path.

For non-clang compilers the user should add option below to
compiler command line to let it find SYCL headers:
-I <path_to_sycl_compiler>/include/sycl

Additional diagnostic can be raised depending on compiler
which is used (see examples for clang below). It can be ignored.

/localdisk2/bbsycl/res/include/sycl/CL/__spirv/spirv_ops.hpp:74:8: error: SYCL 1.2.1 specification does not allow 'sycl_device' attribute applied to a function with a raw pointer parameter type [-Wsycl-strict]
extern SYCL_EXTERNAL __ocl_event_t __spirv_GroupAsyncCopy(

/localdisk2/bbsycl/res/include/sycl/CL/sycl/ordered_queue.hpp:267:37: warning: 'ordered_queue' is deprecated: Replaced by in_order queue property [-Wdeprecated-declarations]
size_t operator()(const cl::sycl::ordered_queue &q) const {

Signed-off-by: Vladimir Lazarev vladimir.lazarev@intel.com

@bader
Copy link
Contributor

bader commented Mar 13, 2020

I suggest moving SYCL header out of compiler headers directory i.e. not put them to lib/clang/11.0.0/include/.
Can we move them to include/sycl directory?

Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

could you please elaborate a bit more about this:

Additional diagnostic can be raised depending on compiler
which is used (see examples for clang below). It can be ignored.
/localdisk2/bbsycl/res/lib/clang/11.0.0/include/sycl/CL/__spirv/spirv_ops.hpp:74:8: error: SYCL 1.2.1 specification does not allow 'sycl_device' attribute applied to a function with a raw pointer parameter type [-Wsycl-strict]
extern SYCL_EXTERNAL __ocl_event_t __spirv_GroupAsyncCopy(
/localdisk2/bbsycl/res/lib/clang/11.0.0/include/sycl/CL/sycl/ordered_queue.hpp:267:37: warning: 'ordered_queue' is deprecated: Replaced by in_order queue property [-Wdeprecated-declarations]
size_t operator()(const cl::sycl::ordered_queue &q) const {

sycl/test/basic_tests/buffer/buffer_interop.cpp Outdated Show resolved Hide resolved
@vladimirlaz vladimirlaz force-pushed the private/vlazarev/move_headers branch from c5650e8 to 662aa1c Compare March 13, 2020 12:50
@vladimirlaz
Copy link
Contributor Author

vladimirlaz commented Mar 13, 2020

could you please elaborate a bit more about this:

Additional diagnostic can be raised depending on compiler
which is used (see examples for clang below). It can be ignored.
/localdisk2/bbsycl/res/lib/clang/11.0.0/include/sycl/CL/__spirv/spirv_ops.hpp:74:8: error: SYCL 1.2.1 specification does not allow 'sycl_device' attribute applied to a function with a raw pointer parameter type [-Wsycl-strict]
extern SYCL_EXTERNAL __ocl_event_t __spirv_GroupAsyncCopy(
/localdisk2/bbsycl/res/lib/clang/11.0.0/include/sycl/CL/sycl/ordered_queue.hpp:267:37: warning: 'ordered_queue' is deprecated: Replaced by in_order queue property [-Wdeprecated-declarations]
size_t operator()(const cl::sycl::ordered_queue &q) const {

As far as I understood compiler warnings are suppressed for system headers (found in path pointed by -internal-isystem). Once the path is added with -I (this is true if the user uses non-sycl compiler for building host code) extra warnings were generated by the compiler. I mentined here examples from LIT execution.
It is up to user to suppress them.

@vladimirlaz
Copy link
Contributor Author

I suggest moving SYCL header out of compiler headers directory i.e. not put them to lib/clang/11.0.0/include/.
Can we move them to include/sycl directory?

Both locations are possible but I prefer current location because:

  • I did not find good way to detect compiler root directory in clang driver. I will need to use .. symbols which might be error prone if there are symbolic links in the path. Current approach adds sub-folder which seems to be safer.
  • There was a request to preserve high level layout to decrease impact on tools and infrastructure.

@vladimirlaz vladimirlaz force-pushed the private/vlazarev/move_headers branch from 662aa1c to c1859ae Compare March 13, 2020 13:46
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

I suggest moving SYCL header out of compiler headers directory i.e. not put them to lib/clang/11.0.0/include/.
Can we move them to include/sycl directory?

Both locations are possible but I prefer current location because:

  • I did not find good way to detect compiler root directory in clang driver. I will need to use .. symbols which might be error prone if there are symbolic links in the path. Current approach adds sub-folder which seems to be safer.

Please take a look how standard library is handled.

  • There was a request to preserve high level layout to decrease impact on tools and infrastructure.

This is a terrible reason to implement error-prone design.

@vladimirlaz vladimirlaz force-pushed the private/vlazarev/move_headers branch from c1859ae to af03f73 Compare March 13, 2020 15:43
@vzakhari
Copy link
Contributor

I suggest moving SYCL header out of compiler headers directory i.e. not put them to lib/clang/11.0.0/include/.
Can we move them to include/sycl directory?

Both locations are possible but I prefer current location because:

  • I did not find good way to detect compiler root directory in clang driver. I will need to use .. symbols which might be error prone if there are symbolic links in the path. Current approach adds sub-folder which seems to be safer.

Please take a look how standard library is handled.

  • There was a request to preserve high level layout to decrease impact on tools and infrastructure.

This is a terrible reason to implement error-prone design.

Do we want to keep versioning for sycl header files the same way clang does for its header files? If yes, should the sycl header files' version match the clang version?

@bader
Copy link
Contributor

bader commented Mar 13, 2020

I suggest moving SYCL header out of compiler headers directory i.e. not put them to lib/clang/11.0.0/include/.
Can we move them to include/sycl directory?

Both locations are possible but I prefer current location because:

  • I did not find good way to detect compiler root directory in clang driver. I will need to use .. symbols which might be error prone if there are symbolic links in the path. Current approach adds sub-folder which seems to be safer.

Please take a look how standard library is handled.

  • There was a request to preserve high level layout to decrease impact on tools and infrastructure.

This is a terrible reason to implement error-prone design.

Do we want to keep versioning for sycl header files the same way clang does for its header files? If yes, should the sycl header files' version match the clang version?

Maybe, but I don't think that moving library headers to the compiler sub-directory is the right approach or has any connection to the library versioning.
SYCL runtime library is a separate project and I prefer treating it that way.
E.g. libc++ is another LLVM project, which versioning following LLVM scheme, but libc++ headers are not installed into compiler built-in headers sub-directory. libc++ project uses CMake utilities to align versioning with the rest of the project and we might do the same.

@vladimirlaz vladimirlaz requested review from bader and Fznamznon March 13, 2020 18:49
@bader
Copy link
Contributor

bader commented Mar 13, 2020

@vladimirlaz, please, resolve merge conflict and fix tests.

@vladimirlaz vladimirlaz force-pushed the private/vlazarev/move_headers branch from af03f73 to d9097e7 Compare March 13, 2020 19:26
@vladimirlaz
Copy link
Contributor Author

@vladimirlaz, please, resolve merge conflict and fix tests.

done

SYCL headers were moved from default clang include directory
(lib/clang/11.0.0/include) to (include/sycl) to support mixed
compilers (e.g. gcc for host code and clang for device code)
during build of SYCL application.

Clang driver adds new directory to system include search path.

For non-clang compilers the user should add option below to
compiler command line to let it find SYCL headers:
-I <path_to_sycl_compiler>/include/sycl

Additional diagnostic can be raised depending on compiler
which is used (see examples for clang below). It can be ignored.

/localdisk2/bbsycl/res/include/sycl/CL/__spirv/spirv_ops.hpp:74:8: error: SYCL 1.2.1 specification does not allow 'sycl_device' attribute applied to a function with a raw pointer parameter type [-Wsycl-strict]
  extern SYCL_EXTERNAL __ocl_event_t __spirv_GroupAsyncCopy(

/localdisk2/bbsycl/res/include/sycl/CL/sycl/ordered_queue.hpp:267:37: warning: 'ordered_queue' is deprecated: Replaced by in_order queue property [-Wdeprecated-declarations]
  size_t operator()(const cl::sycl::ordered_queue &q) const {

Signed-off-by: Vladimir Lazarev vladimir.lazarev@intel.com
@vladimirlaz vladimirlaz force-pushed the private/vlazarev/move_headers branch from d9097e7 to 6642d84 Compare March 14, 2020 04:35
mdtoguchi
mdtoguchi previously approved these changes Mar 14, 2020
@bader
Copy link
Contributor

bader commented Mar 14, 2020

@vladimirlaz, OpenCL headers not found during CTS compilation.

bader
bader previously approved these changes Mar 15, 2020
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

LGTM with a couple of nits.

Please, rename a couple of CMake variables.

sycl/CMakeLists.txt Outdated Show resolved Hide resolved
sycl/CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: Vladimir Lazarev <vladimir.lazarev@intel.com>
@vladimirlaz vladimirlaz dismissed stale reviews from bader and mdtoguchi via fdc0d28 March 15, 2020 06:08
@bader bader merged commit 39501f6 into intel:sycl Mar 15, 2020
@vladimirlaz vladimirlaz deleted the private/vlazarev/move_headers branch March 15, 2020 18:03
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Mar 17, 2020
…e_api_test

* origin/sycl: (1188 commits)
  [SYCL][CUDA] Improve CUDA backend documentation (intel#1293)
  [SYCL] Emit textual IR when -S -fsycl-device-only is used (intel#1314)
  [SYCL] Add prototype of group algorithms (intel#1236)
  [SYCL] XFAIL test on windows to unblock pulldown
  Allow Intel Loop Controls only with SPV_INTEL_fpga_loop_controls
  Apply suggested assert msg change
  Implement SPV_INTEL_io_pipes extension
  [SYCL] Fix dependencies for SYCLLowerIR (intel#1321)
  [CI] Allow builds without pre-downloaded OpenCL in configure.py (intel#1317)
  [SYCL] Move SYCL headers from standard clang location (intel#1308)
  [mlir] Add support for generating dialect declarations via tablegen.
  Be more strict when checking existence of foo
  [CodeGenPrepare] Freeze condition when transforming select to br
  [ORC] Remove an undefined static method from LLJIT.
  [JITLink][AArch64] Fix incorrect capitalization in a testcase name.
  [ORC] Print symbol flags and materializer name in ExecutionSession::dump.
  [JITLink][MachO] Re-apply b64afad, MachO linker-private support, with fixes.
  Basic Block Sections Support.
  Test commit.
  [SYCL][Doc] Deploy documentation for PI (intel#1318)
  ...
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Feb 23, 2023
Depending upon device capabilities, global/local sizes in ndrange may vary considerably. This turned up in testing on an older Sandy Bridge machine. The reduction, itself, is working fine on all places.
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.

6 participants