Skip to content

[SYCL] Add 'cl::sycl::intel::experimental::printf' #835

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 27, 2019
Merged

[SYCL] Add 'cl::sycl::intel::experimental::printf' #835

merged 1 commit into from
Dec 27, 2019

Conversation

AlexeySachkov
Copy link
Contributor

@AlexeySachkov AlexeySachkov commented Nov 14, 2019 •
edited
Loading

New built-in is mapped to OpenCL 'printf' built-in by using SPIR-V
friendly IR.

Fznamznon
Fznamznon reviewed Nov 14, 2019
View reviewed changes
romanovvlad
romanovvlad reviewed Nov 14, 2019
View reviewed changes
asavonic
asavonic suggested changes Nov 14, 2019
View reviewed changes
@asavonic
Copy link
Contributor

asavonic commented Dec 5, 2019

Another approach for the changes in clang: #909

@asavonic asavonic mentioned this pull request Dec 5, 2019
Merged
@AlexeySachkov AlexeySachkov force-pushed the private/asachkov/printf branch from f91daff to 52372fd Compare December 23, 2019 15:10
@AlexeySachkov AlexeySachkov changed the title [SYCL] Add 'cl::sycl::intel::printf' [SYCL] Add 'cl::sycl::intel::experimental::printf' Dec 23, 2019
AlexeySachkov
AlexeySachkov commented Dec 23, 2019
View reviewed changes
@AlexeySachkov AlexeySachkov force-pushed the private/asachkov/printf branch from 52372fd to 198bce3 Compare December 24, 2019 13:15
@AlexeySachkov AlexeySachkov assigned asavonic and unassigned Fznamznon Dec 24, 2019
@AlexeySachkov AlexeySachkov force-pushed the private/asachkov/printf branch 3 times, most recently from 29a9b9c to 85bc165 Compare December 25, 2019 17:15
asavonic
asavonic suggested changes Dec 26, 2019
View reviewed changes
sycl/test/built-ins/printf.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fragile, even for a workaround. I hope this bug is stable
enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From local runs it seems that it is stable enough, yes

@AlexeySachkov AlexeySachkov force-pushed the private/asachkov/printf branch from 85bc165 to 5f245ca Compare December 26, 2019 12:30
keryell
keryell reviewed Dec 26, 2019
View reviewed changes
sycl/include/CL/sycl/intel/builtins.hpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than non-standard, there are mostly Intel extensions, aren't they?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, at this point I would say it mostly "non-standard" rather than actual Intel extensions, since this file is only contains experimental::printf which doesn't have special extension document

@AlexeySachkov AlexeySachkov force-pushed the private/asachkov/printf branch from 5f245ca to e18f7ff Compare December 26, 2019 14:21
asavonic
asavonic reviewed Dec 26, 2019
View reviewed changes
Copy link
Contributor

@asavonic asavonic left a comment

Choose a reason for hiding this comment

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

LGTM.

sycl/include/CL/sycl/intel/builtins.hpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

And if I have cl::sycl::vec<long, 8> on Windows, I have to use %v8d instead of %v8ld, because long is 32bit there, right? We definitely need a correct format diagnostic here (not with this patch). Or maybe Clang can adjust a format string according to the OpenCL rules, since it is guaranteed to be a compile-time constant.

@AlexeySotkin @AlexeySachkov
[SYCL] Add 'cl::sycl::intel::experimental::printf'
1db211f 
New built-in is mapped to OpenCL 'printf' built-in by using SPIR-V
friendly IR.

Signed-off-by: Alexey Sotkin <alexey.sotkin@intel.com>
Co-authored-by: Alexey Sachkov <alexey.sachkov@intel.com>
@AlexeySachkov AlexeySachkov force-pushed the private/asachkov/printf branch from e18f7ff to 1db211f Compare December 27, 2019 10:17
@romanovvlad romanovvlad merged commit 78d80a1 into intel:sycl Dec 27, 2019
@AlexeySachkov AlexeySachkov deleted the private/asachkov/printf branch December 27, 2019 15:59
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
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>
@AlexeySachkov AlexeySachkov mentioned this pull request Jan 20, 2020
Merged
@bader bader mentioned this pull request Feb 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fznamznon Fznamznon Fznamznon left review comments

@romanovvlad romanovvlad romanovvlad left review comments

@rolandschulz rolandschulz Awaiting requested review from rolandschulz

@mkinsner mkinsner Awaiting requested review from mkinsner

+3 more reviewers

@keryell keryell keryell left review comments

@agozillon agozillon agozillon left review comments

@asavonic asavonic asavonic approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@asavonic asavonic

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@AlexeySachkov @asavonic @keryell @agozillon @Fznamznon @romanovvlad @AlexeySotkin