[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-05-21 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon updated this revision to Diff 200513. Fznamznon added a comment. Herald added a subscriber: mgorny. Added semantics for new attributes - Added semantics for new attributes. Now complier can separate SYCL device code from host code using new arrtributes. - Removed spelling for

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-27 Thread Anastasia Stulova via Phabricator via cfe-commits
Anastasia added a comment. I think potentially reusing OpenCL features is desirable since the device code of SYCL is largely OpenCL. However I don't think we are clear enough about the overall device compilation flow of SYCL and I can easily suggest a number of different approaches including

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-19 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added a comment. I tried to reuse OpenCL kernel attribute with "__kernel" keyword in our current SYCL implementation. PR with this try is here - https://github.com/intel/llvm/pull/97 Now It looks feasible but with a couple notes: From SYCL specification "SYCL is designed to be as

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-19 Thread Anastasia Stulova via Phabricator via cfe-commits
Anastasia added a comment. > SYCL is similar to OpenMP 5 for C++, where you use only C++ classes instead > of #pragma. So it is quite C++-friendlier than OpenMP. I am not sure what you mean by friendlier? OpenMP concept is very clear to be - a program written in C/C++ or Fortran can be

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-18 Thread Ronan Keryell via Phabricator via cfe-commits
keryell added a comment. In D60455#1470402 , @Anastasia wrote: > > In the interest of speeding up the upstreaming work, would you be able to > highlight the differences and similarity at least for SYCL and OpenCL kernel > modes? Not sure if you are

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-17 Thread Ronan Keryell via Phabricator via cfe-commits
keryell added a comment. In D60455#1470030 , @Anastasia wrote: > > I am not sure we need to add a keyword actually, the attribute can just be > added in AST since it's not supposed to be used in the source code? The attribute is used by the SYCL

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-17 Thread Anastasia Stulova via Phabricator via cfe-commits
Anastasia added a comment. > > >> I think what we are missing currently is a thorough analysis/comparison >> between SYCL device mode and OpenCL kernel language mode to understand >> what's the best implementation strategy. That would apply to many other >> features: kernel function

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-17 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added a comment. In D60455#1470318 , @keryell wrote: > It would be better to rename `clang/test/SemaSYCL/device-attrubutes.cpp` to > `clang/test/SemaSYCL/device-attributes.cpp` It's already renamed in the latest patch. Repository: rG LLVM

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-17 Thread Ronan Keryell via Phabricator via cfe-commits
keryell added a comment. It would be better to rename `clang/test/SemaSYCL/device-attrubutes.cpp` to `clang/test/SemaSYCL/device-attributes.cpp` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60455/new/ https://reviews.llvm.org/D60455

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-17 Thread Anastasia Stulova via Phabricator via cfe-commits
Anastasia added a comment. In D60455#1468386 , @Fznamznon wrote: > In D60455#1467018 , @Anastasia wrote: > > > Just to understand how this will work. I would imagine you can have a > > device function definition

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-17 Thread Anastasia Stulova via Phabricator via cfe-commits
Anastasia added a comment. In D60455#1469150 , @aaron.ballman wrote: > In D60455#1468714 , @bader wrote: > > > In D60455#1468386 , @Fznamznon > > wrote: > > > > > > Ok, my

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-16 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In D60455#1468714 , @bader wrote: > In D60455#1468386 , @Fznamznon wrote: > > > > Ok, my question is whether you are planning to duplicate the same logic > > > as for OpenCL kernel

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-16 Thread Alexey Bader via Phabricator via cfe-commits
bader added a comment. In D60455#1467279 , @aaron.ballman wrote: > In D60455#1464324 , @Fznamznon wrote: > > > Applied comments from @aaron.ballman and @keryell > > > > - Introduced a C++11 and C2x style spelling

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-16 Thread Alexey Bader via Phabricator via cfe-commits
bader added a comment. In D60455#1468386 , @Fznamznon wrote: > > Ok, my question is whether you are planning to duplicate the same logic as > > for OpenCL kernel which doesn't really seem like an ideal design choice. Is > > this the only difference then

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-16 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added a comment. In D60455#1467279 , @aaron.ballman wrote: > > I'm still wondering what the actual semantics are for the attribute. Right > now, these are being parsed and ignored -- are there future plans here? Yes, we are going to teach

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-16 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added inline comments. Comment at: clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp:2 +// RUN: %clang_cc1 -fsyntax-only -fsycl-is-device -verify %s +// Now pretend that we're compiling a C++ file. There should be warnings. +// RUN: %clang_cc1 -fsyntax-only -verify

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-16 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:286 +help. + }]; +} Anastasia wrote: > keryell wrote: > > aaron.ballman wrote: > > > I'm still not entirely certain how I would know what to mark and how. > > > From the

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-16 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added a comment. In D60455#1467018 , @Anastasia wrote: > Just to understand how this will work. I would imagine you can have a device > function definition preceding its use. When the function is being parsed it's > not known yet whether it

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-16 Thread Anastasia Stulova via Phabricator via cfe-commits
Anastasia added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:286 +help. + }]; +} keryell wrote: > aaron.ballman wrote: > > I'm still not entirely certain how I would know what to mark and how. From > > the description, it sounds like

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-15 Thread Ronan Keryell via Phabricator via cfe-commits
keryell accepted this revision. keryell added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1000 +def SYCLDevice : InheritableAttr { + let Spellings = [GNU<"sycl_device">]; + let Subjects = SubjectList<[Function, Var]>; aaron.ballman wrote: >

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-15 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. In D60455#1464324 , @Fznamznon wrote: > Applied comments from @aaron.ballman and @keryell > > - Introduced a C++11 and C2x

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-15 Thread Anastasia Stulova via Phabricator via cfe-commits
Anastasia added a comment. > Our current approach is to add an attribute, which SYCL runtime will use to > mark code passed to `cl::sycl::handler::parallel_for` as "kernel functions". > Obviously runtime library can't mark `foo` as "device" code - this is a > compiler job: to traverse all

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-15 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon updated this revision to Diff 195134. Fznamznon added a comment. Applied comment from @bader. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60455/new/ https://reviews.llvm.org/D60455 Files: clang/include/clang/Basic/Attr.td

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-15 Thread Alexey Bader via Phabricator via cfe-commits
bader accepted this revision. bader added a comment. LGTM. One minor comment. Comment at: clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp:3 +// Now pretend that we're compiling a C++ file. There should be warnings. +// RUN: %clang_cc1 -DEXPECT_WARNINGS -fsyntax-only

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-15 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon updated this revision to Diff 195128. Fznamznon added a comment. Applied comments from @bader Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60455/new/ https://reviews.llvm.org/D60455 Files: clang/include/clang/Basic/Attr.td

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-15 Thread Alexey Bader via Phabricator via cfe-commits
bader added inline comments. Comment at: clang/test/SemaSYCL/device-attrubutes.cpp:8-11 +__attribute((sycl_kernel)) void foo(); +__attribute((sycl_device)) void foo1(); +[[clang::sycl_kernel]] void foo2(); +[[clang::sycl_device]] void foo3(); Fznamznon wrote: >

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-15 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added inline comments. Comment at: clang/test/SemaSYCL/device-attrubutes.cpp:8-11 +__attribute((sycl_kernel)) void foo(); +__attribute((sycl_device)) void foo1(); +[[clang::sycl_kernel]] void foo2(); +[[clang::sycl_device]] void foo3(); bader wrote: >

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-12 Thread Alexey Bader via Phabricator via cfe-commits
bader accepted this revision. bader added inline comments. Comment at: clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp:5 + +#if defined(EXPECT_WARNINGS) +// expected-warning@+8 {{'sycl_kernel' attribute ignored}} I think you can use `__SYCL_DEVICE_ONLY__`

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-12 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon updated this revision to Diff 194878. Fznamznon added a comment. Applied comments from @aaron.ballman and @keryell - Introduce a C++11 and C2x style spelling in the clang namespace I didn't find path to add two namespaces to attribute (like [[clang::sycl::device]] so

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-11 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1000 +def SYCLDevice : InheritableAttr { + let Spellings = [GNU<"sycl_device">]; + let Subjects = SubjectList<[Function, Var]>; keryell wrote: > Fznamznon wrote: > >

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-09 Thread Ronan Keryell via Phabricator via cfe-commits
keryell added a comment. While we are thinking about recycling some attributes across languages, we have to think about the fact that real applications are often made from various languages, such as SYCL + OpenMP 5 or whatever. It would be nice not to forbid such a combination inside the same

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-09 Thread Alexey Bader via Phabricator via cfe-commits
bader added a comment. To give more context to the question, I'd like to clarify the use case of new attributes. As @Fznamznon already mentioned in previous comments SYCL is not supposed to expose any non-standard extensions to a user. Here is code example of the SYCL program, which

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-09 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added a comment. In D60455#1459935 , @Anastasia wrote: > I was just wondering since SYCL is intended to be a single source C++ for > OpenCL and this attribute is for internal use is it possible to just reuse > existing OpenCL kernel attribute?

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-09 Thread Anastasia Stulova via Phabricator via cfe-commits
Anastasia requested changes to this revision. Anastasia added a comment. This revision now requires changes to proceed. I was just wondering since SYCL is intended to be a single source C++ for OpenCL and this attribute is for internal use is it possible to just reuse existing OpenCL kernel

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-09 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1000 +def SYCLDevice : InheritableAttr { + let Spellings = [GNU<"sycl_device">]; + let Subjects = SubjectList<[Function, Var]>; aaron.ballman wrote: > Is there a reason to not also

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-09 Thread Alexey Bataev via Phabricator via cfe-commits
ABataev added a comment. In D60455#1459678 , @Fznamznon wrote: > Hi all, > > I assume that something to mark the code which is supposed to be offloaded > already is implemented for OpenMP and CUDA. For example I found CUDA-specific > CUDADevice

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-09 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon added a comment. Hi all, I assume that something to mark the code which is supposed to be offloaded already is implemented for OpenMP and CUDA. For example I found CUDA-specific CUDADevice attribute. I can't just use CUDA-specific attributes for SYCL but I think that these resources

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-09 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added a comment. These attributes are being parsed and silently ignored -- are there semantics expected for the attributes? Comment at: clang/include/clang/Basic/Attr.td:1000 +def SYCLDevice : InheritableAttr { +

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

2019-04-09 Thread Mariya Podchishchaeva via Phabricator via cfe-commits
Fznamznon created this revision. Herald added subscribers: cfe-commits, Anastasia, ebevhan. Herald added a project: clang. SYCL runtime is supposed to use sycl_kernel attribute to mark functions in the single source which are supposed to be compiled for the device. Compiler is supposed to