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

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 sycl_dev

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

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 th

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

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 clos

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

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 complime

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

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 f

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

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 head

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

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 restric

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

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

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

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 pr

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

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

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 wh

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

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 i

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

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

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

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

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 descript

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

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 wil

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

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 whoev

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

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: > a

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

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 style

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

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 sym

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

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 clang/inc

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

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 -veri

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

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 clang/inc

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

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: > b

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

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: > T

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

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__` d

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

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 [[clang::sycl_dev

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

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: > > aaron.ballma

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

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 TU

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

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 demonstrat

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

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

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 attr

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

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 i

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

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 attribute

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

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

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 { + le

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

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 understa