[NFC][SYCL] Move include/CL/__spirv -> include/sycl/__spirv
#15515
Conversation
aelovikov-intel
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
|
This is a follow-up for the discussion in #15437 (comment). |
aelovikov-intel
force-pushed
the
move__spirv
branch
from
24698a9 to
b9335e2
Compare
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| @@ -172,7 +172,7 @@ namespace __spirv { | |||
| // Helper function templates to initialize and get vector component from SPIR-V | |||
| // built-in variables | |||
| #define __SPIRV_DEFINE_INIT_AND_GET_HELPERS(POSTFIX) \ | |||
| template <int ID> static size_t get##POSTFIX(); \ | |||
| template <int ID> size_t get##POSTFIX(); \ | |||
There was a problem hiding this comment.
This is not related to the move, therefore should go in a separate PR.
There was a problem hiding this comment.
It is - if I move without this, the build fails. I'm not sure why warning isn't emitted without the move but that's not that important.
There was a problem hiding this comment.
This is a functional change.
There was a problem hiding this comment.
I'm not sure why warning isn't emitted without the move but that's not that important.
Kind of disagree. To see different diagnostics by just renaming the file sounds important to investigate.
There was a problem hiding this comment.
I think that would be a waste of time, so if that investigation is required for making the change in this PR, I'd rather focus on other things to do.
There was a problem hiding this comment.
I checked it myself and it seems like a bug in sycl/test/warnings/warnings.cpp checks warnings only in sycl directory and ignores warnings from the headers located in other directories. Adding --no-system-header-prefix=CL to https://github.com/intel/llvm/blob/sycl/sycl/test/warnings/warnings.cpp#L1 reproduces the problem with existing file location.
There was a problem hiding this comment.
In that case, the warning is legit and we have to fix it here. I don't think adding the option to the test is desirable because we want to move our headers out of CL/ instead. Also, we don't have control over OpenCL headers that reside there.
There was a problem hiding this comment.
This is a loophole in logic that defeats the purpose of the test. Headers from sycl/ include headers from CL/ and std/ directories, whereas test only checks for warnings in sycl/ directory. We can pass this test, but still have warnings in the code.
|
Reviewers, ping. |
|
Reviewers ping: @intel/dpcpp-esimd-reviewers @intel/llvm-reviewers-cuda @intel/llvm-reviewers-runtime |
|
Reviewers ping x3: @intel/llvm-reviewers-cuda @intel/llvm-reviewers-runtime |
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
aelovikov-intel
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
No description provided.