[SYCL] Don't add `include/sycl/` to the system includes path #15437

aelovikov-intel · 2024-09-18T22:25:21Z

Open question: I'm not sure if https://github.com/intel/llvm/tree/sycl/sycl/include/CL/__spirv should go into include/CL/__spirv or somewhere else. On the other hand, https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl.hpp has to be installed into include/CL/sycl.hpp per the specification.

Fixes intel#6770

sarnex

esimd changes lgtm, will let others review the overall change

mdtoguchi · 2024-09-19T20:12:14Z

Driver changes look fine - seeing the number of header inclusion changes we have had to make internally - how do we minimize impact on users?

bader · 2024-09-19T20:13:16Z

Fixes #6770

Thanks for fixing this long-standing issue!

On the other hand, https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl.hpp has to be installed into include/CL/sycl.hpp per the specification.

The spec says that

For compatibility with SYCL 1.2.1, SYCL provides another standard header file: <CL/sycl.hpp>, which can be included in place of <sycl/sycl.hpp>. In that case, all SYCL classes, constants, types and functions defined by this specification should exist within the ::cl::sycl C++ namespace.

It doesn't require implementation to put it into include directory. Moreover, it doesn't mandate the driver to add additional search paths for SYCL headers. It's DPC++ implementation choice.

I'm not sure if https://github.com/intel/llvm/tree/sycl/sycl/include/CL/__spirv should go into include/CL/__spirv or somewhere else.

What is your concern about the __spirv directory location?

aelovikov-intel · 2024-09-19T20:17:26Z

It doesn't require implementation to put it into include directory. Moreover, it doesn't mandate the driver to add additional search paths for SYCL headers. It's DPC++ implementation choice.

Yeah, I wasn't precise in what I said. What I meant is that #include <CL/sycl.hpp> must work and now that we don't put include/sycl in search path we have to move that header.

aelovikov-intel · 2024-09-19T20:18:40Z

What is your concern about the __spirv directory location?

It feels awkward to me that our implementation detail is located in OpenCL-specific folder. It might better be suited for sycl/__spirv.

aelovikov-intel · 2024-09-19T20:20:37Z

Driver changes look fine - seeing the number of header inclusion changes we have had to make internally - how do we minimize impact on users?

My personal opinion: we don't - it was a bug and now it's fixed.

bader · 2024-09-19T20:27:07Z

What is your concern about the __spirv directory location?

It feels awkward to me that our implementation detail is located in OpenCL-specific folder. It might better be suited for sycl/__spirv.

Agree. SYCL-1.2.1 implementation put all headers into CL directory. When we added support for SYCL-2020, we moved all headers from CL to sycl directory. It sounds like it's a leftover of this work. I expect CL to have only one file - sycl.hpp, which includes sycl/sycl.hpp and imports all SYCL declarations to cl namespace.

aelovikov-intel · 2024-09-19T20:35:15Z

What is your concern about the __spirv directory location?

It feels awkward to me that our implementation detail is located in OpenCL-specific folder. It might better be suited for sycl/__spirv.

Agree. SYCL-1.2.1 implementation put all headers into CL directory. When we added support for SYCL-2020, we moved all headers from CL to sycl directory. It sounds like it's a leftover of this work. I expect CL to have only one file - sycl.hpp, which includes sycl/sycl.hpp and imports all SYCL declarations to cl namespace.

Good! I'll address it separately after this PR is merged in.

uditagarwal97

SYCL RT changes LGTM

AlexeySachkov · 2024-09-20T08:11:24Z

sycl/include/sycl/accessor.hpp

@@ -37,7 +37,7 @@
 #include <sycl/property_list.hpp>                     // for property_list
 #include <sycl/range.hpp>                             // for range
 #include <sycl/sampler.hpp>                           // for addressing_mode
-#include <ur_api.h>                                   // for UR_RESULT_ERRO...
+#include <sycl/ur_api.h>                              // for UR_RESULT_ERRO...


This change makes me think that ur_api.h is our own header, but that's not true. I wonder if we need to put it into some _deps subfolder for clarity.

I've been thinking about this too. I think even sycl/_deps/ur_api.h might be questionable, because one might want to use both UR and SYCL on the application side and I'd expect user's UR must somehow match the one libsycl.so was built with. Maybe I should be deploying these ur_*.h/.hpp at root include/ directory instead.

Does anybody have any ideas here?

Implemented the move in the latest revision.

Or should it be ur/ur_*.h ? @intel/unified-runtime-reviewers

Ping @intel/unified-runtime-reviewers to answer the question above. @AlexeySachkov just in case as well.

I don't think we ever intend for UR to be used directly by an application so we can eliminate that as a concern. In the UR repo the API header just lives in the root include so ur/ur_api.h would look a little weird from that perspective

If UR is not intended to be used by end users, then I think some generic _deps is good enough and it will be less confusing than ur/ for those who work with it.

I'm not sure if I prefer _deps/ur_api.h vs just ur_api.h, especially since the latter is the prior spelling of the includes. As such, _deps/* move should be in another PR, if at all.

joeatodd

SYCLcompat changes LGTM

kbenzie

UR LGTM

aelovikov-intel · 2024-09-24T18:45:38Z

Ping @intel/bindless-images-reviewers .

Seanst98

Bindless LGTM

[SYCL] Don't add include/sycl/ to the system includes path

c84710c

Fixes intel#6770

aelovikov-intel had a problem deploying to WindowsCILock September 18, 2024 22:25 — with GitHub Actions Failure

aelovikov-intel had a problem deploying to WindowsCILock September 18, 2024 23:08 — with GitHub Actions Failure

Fix failing tests

8eaa803

aelovikov-intel had a problem deploying to WindowsCILock September 19, 2024 17:19 — with GitHub Actions Failure

aelovikov-intel temporarily deployed to WindowsCILock September 19, 2024 17:54 — with GitHub Actions Inactive

Try to fix two other tests

c995635

aelovikov-intel temporarily deployed to WindowsCILock September 19, 2024 18:52 — with GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock September 19, 2024 19:33 — with GitHub Actions Inactive

aelovikov-intel marked this pull request as ready for review September 19, 2024 19:58

aelovikov-intel requested review from a team as code owners September 19, 2024 19:58

aelovikov-intel requested review from uditagarwal97, bader, AlexeySachkov and steffenlarsen September 19, 2024 19:58

sarnex approved these changes Sep 19, 2024

View reviewed changes

uditagarwal97 approved these changes Sep 19, 2024

View reviewed changes

PietroGhg approved these changes Sep 20, 2024

View reviewed changes

AlexeySachkov reviewed Sep 20, 2024

View reviewed changes

mdtoguchi approved these changes Sep 23, 2024

View reviewed changes

Move ur_* headers to the top level include/

00b70cd

aelovikov-intel had a problem deploying to WindowsCILock September 23, 2024 19:19 — with GitHub Actions Error

Minor style fix

9d2a837

aelovikov-intel requested a review from AlexeySachkov September 23, 2024 19:22

aelovikov-intel had a problem deploying to WindowsCILock September 23, 2024 19:22 — with GitHub Actions Failure

Revert changes in sycl/source/detail/device_binary_image.hpp

9e05e3b

aelovikov-intel had a problem deploying to WindowsCILock September 23, 2024 20:07 — with GitHub Actions Error

FIx an e2e test

1f8e2ec

aelovikov-intel had a problem deploying to WindowsCILock September 23, 2024 20:08 — with GitHub Actions Failure

aelovikov-intel had a problem deploying to WindowsCILock September 23, 2024 21:21 — with GitHub Actions Failure

joeatodd approved these changes Sep 24, 2024

View reviewed changes

kbenzie approved these changes Sep 24, 2024

View reviewed changes

Merge remote-tracking branch 'origin/sycl' into HEAD

209ff1d

aelovikov-intel temporarily deployed to WindowsCILock September 24, 2024 17:09 — with GitHub Actions Inactive

aelovikov-intel temporarily deployed to WindowsCILock September 24, 2024 18:36 — with GitHub Actions Inactive

Seanst98 approved these changes Sep 25, 2024

View reviewed changes

aelovikov-intel merged commit d640ff4 into intel:sycl Sep 25, 2024
12 checks passed

aelovikov-intel deleted the fix-include-path branch September 25, 2024 16:10

aelovikov-intel mentioned this pull request Sep 25, 2024

[NFC][SYCL] Move include/CL/__spirv -> include/sycl/__spirv #15515

Merged

pbalcer mentioned this pull request Oct 7, 2024

Fix of <sycl.hpp> includes oneapi-src/Velocity-Bench#76

Merged

bader mentioned this pull request Oct 24, 2024

[Driver][SYCL] Add initial SYCL offload compilation support llvm/llvm-project#107493

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Don't add `include/sycl/` to the system includes path #15437

[SYCL] Don't add `include/sycl/` to the system includes path #15437

aelovikov-intel commented Sep 18, 2024 •

edited

Loading

sarnex left a comment

mdtoguchi commented Sep 19, 2024

bader commented Sep 19, 2024

aelovikov-intel commented Sep 19, 2024

aelovikov-intel commented Sep 19, 2024

aelovikov-intel commented Sep 19, 2024

bader commented Sep 19, 2024

aelovikov-intel commented Sep 19, 2024

uditagarwal97 left a comment

AlexeySachkov Sep 20, 2024

aelovikov-intel Sep 23, 2024

aelovikov-intel Sep 23, 2024

aelovikov-intel Sep 23, 2024

aelovikov-intel Sep 24, 2024

aarongreig Sep 25, 2024

AlexeySachkov Sep 25, 2024

aelovikov-intel Sep 25, 2024

joeatodd left a comment

kbenzie left a comment

aelovikov-intel commented Sep 24, 2024

Seanst98 left a comment

[SYCL] Don't add include/sycl/ to the system includes path #15437

[SYCL] Don't add include/sycl/ to the system includes path #15437

Conversation

aelovikov-intel commented Sep 18, 2024 • edited Loading

sarnex left a comment

Choose a reason for hiding this comment

mdtoguchi commented Sep 19, 2024

bader commented Sep 19, 2024

aelovikov-intel commented Sep 19, 2024

aelovikov-intel commented Sep 19, 2024

aelovikov-intel commented Sep 19, 2024

bader commented Sep 19, 2024

aelovikov-intel commented Sep 19, 2024

uditagarwal97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeatodd left a comment

Choose a reason for hiding this comment

kbenzie left a comment

Choose a reason for hiding this comment

aelovikov-intel commented Sep 24, 2024

Seanst98 left a comment

Choose a reason for hiding this comment

[SYCL] Don't add `include/sycl/` to the system includes path #15437

[SYCL] Don't add `include/sycl/` to the system includes path #15437

aelovikov-intel commented Sep 18, 2024 •

edited

Loading