Skip to content

[SYCL] Add __SYCL_INLINE attribute to cl namespace in library sources#986

Merged
bader merged 3 commits intointel:syclfrom
bader:sycl-namespace-inline
Jan 9, 2020
Merged

[SYCL] Add __SYCL_INLINE attribute to cl namespace in library sources#986
bader merged 3 commits intointel:syclfrom
bader:sycl-namespace-inline

Conversation

@bader
Copy link
Contributor

@bader bader commented Dec 30, 2019

This change aligns class/functions definitions with the declarations in the headers.

bader and others added 3 commits December 30, 2019 16:11
@bader
[SYCL] Add __SYCL_INLINE attribute to cl namespace in library sources
6b70ffd 
This change aligns class/functions definitions with the declarations in
the headers.

Signed-off-by: Alexey Bader <alexey.bader@intel.com>
@bader @romanovvlad
[SYCL][Test] Add test case for using sycl namespace
65a1d8a 
Signed-off-by: Alexey Bader <alexey.bader@intel.com>
Co-authored-by: Vlad Romoanov <vlad.romanov@intel.com>
@bader
[SYCL][NFC] Add copyright header to the library sources
ac04b52 
Signed-off-by: Alexey Bader <alexey.bader@intel.com>
@bader bader force-pushed the sycl-namespace-inline branch from 74367b6 to ac04b52 Compare December 30, 2019 16:16
@bader bader requested a review from romanovvlad December 30, 2019 16:21
@bader bader mentioned this pull request Dec 30, 2019
Merged
@bader bader merged commit f51030c into intel:sycl Jan 9, 2020
@bader bader deleted the sycl-namespace-inline branch January 9, 2020 15:29
@rolandschulz
Copy link
Contributor

rolandschulz commented Jan 10, 2020

This doesn't work if the user sets __SYCL_DISABLE_NAMESPACE_INLINE__, does it? Then things aren't aligned anymore because the library is built different than the user code. Should we remove __SYCL_DISABLE_NAMESPACE_INLINE__?

@bader
Copy link
Contributor Author

bader commented Jan 13, 2020

This doesn't work if the user sets __SYCL_DISABLE_NAMESPACE_INLINE__, does it? Then things aren't aligned anymore because the library is built different than the user code. Should we remove __SYCL_DISABLE_NAMESPACE_INLINE__?

According to my understanding it works. We need to align declarations with definitions at library compilation time to avoid compiler warnings, which are treated as errors. When compiled, these symbols use exposed as fully qualified to the linker, which do not have inline namespace concept - it's used only by the compiler front-end for symbol resolution at compile time.
Let me know if you still have concerns wrt this patch.

AlexeySachkov added a commit to AlexeySachkov/llvm that referenced this pull request Jan 13, 2020
@AlexeySachkov
[SYCL] Add missing __SYCL_INLINE
7daad3b 
One of the missed spots was introduced in intel#835, another one
was missed in intel#986

Signed-off-by: Alexey Sachkov <alexey.sachkov@intel.com>
@AlexeySachkov AlexeySachkov mentioned this pull request Jan 13, 2020
Merged
@rolandschulz
Copy link
Contributor

rolandschulz commented Jan 14, 2020 •
edited
Loading

Let me know if you still have concerns wrt this patch.

No concerns left. I didn't realize that the mangled name is identical between inline and non-inline namespace. Given that this happens to be the case, it should be fine.

bader pushed a commit that referenced this pull request Jan 14, 2020
@AlexeySachkov @bader
[SYCL][NFC] Add missing __SYCL_INLINE (#1003)
8803f62 
One of the missed spots was introduced in #835, another one
was missed in #986

Signed-off-by: Alexey Sachkov <alexey.sachkov@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@romanovvlad romanovvlad romanovvlad approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sergey-semenov sergey-semenov

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bader @rolandschulz @romanovvlad @sergey-semenov