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

[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 sycl_device attribute and its documentation because

it can be added only implicitly by the compiler for now

- Removed docs for sycl_kernel attribute because this attribute is not

presented in SYCL spec and not for public use - it's some implemetation detail.
It will be used in SYCL headers implemetation to help compiler find device code
entry point in single source file. So I think no need to add documentation for
it.


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/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Parse/ParseAST.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/lib/Sema/SemaSYCL.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGenSYCL/device-functions.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp
  clang/test/SemaSYCL/device-attributes.cpp

Index: clang/test/SemaSYCL/device-attributes.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/device-attributes.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -fsycl-is-device -verify %s
+
+[[clang::sycl_kernel]] int gv2 = 0; // expected-warning {{'sycl_kernel' attribute only applies to functions}}
+__attribute((sycl_kernel)) int gv3 = 0; // expected-warning {{'sycl_kernel' attribute only applies to functions}}
+
+__attribute((sycl_kernel)) void foo();
+[[clang::sycl_kernel]] void foo1();
+
+__attribute((sycl_kernel(1))) void foo(); // expected-error {{'sycl_kernel' attribute takes no arguments}}
+[[clang::sycl_kernel(1)]] void foo2(); // expected-error {{'sycl_kernel' attribute takes no arguments}}
Index: clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fsyntax-only -fsycl-is-device -verify %s
+// Now pretend that we're compiling regular C++ file without SYCL mode enabled.
+// There should be warnings.
+// RUN: %clang_cc1 -fsyntax-only -verify -x c++ %s
+
+#if not defined(__SYCL_DEVICE_ONLY__)
+// expected-warning@+6 {{'sycl_kernel' attribute ignored}}
+// expected-warning@+6 {{'sycl_kernel' attribute ignored}}
+#else
+// expected-no-diagnostics
+#endif
+
+__attribute((sycl_kernel)) void foo();
+[[clang::sycl_kernel]] void foo2();
+
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -124,6 +124,7 @@
 // CHECK-NEXT: ReturnTypestate (SubjectMatchRule_function, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ReturnsNonNull (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)
+// CHECK-NEXT: SYCLKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ScopedLockable (SubjectMatchRule_record)
 // CHECK-NEXT: Section (SubjectMatchRule_function, SubjectMatchRule_variable_is_global, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
 // CHECK-NEXT: SetTypestate (SubjectMatchRule_function_is_member)
Index: clang/test/CodeGenSYCL/device-functions.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/device-functions.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -triple spir64-unknown-unknown -std=c++11 -fsycl-is-device -S -emit-llvm %s -o - | FileCheck %s
+
+template 
+T bar(T arg);
+
+void foo() {
+  int a = 1 + 1 + bar(1);
+}
+
+template 
+T bar(T arg) {
+  return arg;
+}
+
+template 
+__attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) {
+  kernelFunc();
+}
+
+int main() {
+  kernel_single_task([]() { foo(); });
+  return 0;
+}
+// CHECK: define spir_func void @{{.*}}foo
+// CHECK: define linkonce_odr spir_func i32 @{{.*}}bar
+// CHECK: define internal spir_func void @{{.*}}kernel_single_task
+// FIXME: Next function is lambda () operator. spir_func calling convention
+// is missed for C++ methods.
+// CHECK: define internal void @"_ZZ4mainENK3$_0clEv"(%class.anon* %this)
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5517,14 +5517,30 @@
 Function, [

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

[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 those that don't modify compiler at all. :)  I 
am afraid until we have the big picture clear it will be hard to make any 
sensible decisions.

I have created an issue about it earlier

https://github.com/intel/llvm/issues/59

and I am going to add some more comments there to explain what we should 
elaborate and agree on.

I suggest we finalize the big picture in the following weeks first and then we 
can go ahead with the detailed work in the reviews.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 close to standard C++ as 
possible. Standard C++ compiler can compile the SYCL programs and they will run 
correctly on host CPU." So SYCL doesn't provide non-standard `kernel` keyword 
which is provided by OpenCL. Due this fact it's not possible to add `kernel` 
keyword as in OpenCL, it will prevent compilation of following valid SYCL code:

  int foo(int kernel) { return ++kernel; } // If "kernel" will be a keyword 
like in OpenCL, here will be a error
  …
  using namespace cl::sycl;
  queue Q;
  buffer a(range<1>{1024});
  Q.submit([&](handler& cgh) {
auto A = a.get_access(cgh);
cgh.parallel_for(range<1>{1024}, [=](id<1> index) {
  A[index] = index[0] * 2 + index[1] + foo(42);
});
  }
  ...

So I added only `__kernel` keyword for SYCL because in C++ identifiers which 
start with `__` are reserved for compiler internals.
Next note:
In our current implementation actually not quite that function which is marked 
with `sycl_kernel` (or `__kernel`, whatever) will be real OpenCL kernel in 
produced module. In SYCL all shared between host and device memory objects 
(buffers/images, these objects map to OpenCL buffers and images) can be 
accessed through special `accessor` classes. SYCL also has special mechanism 
for passing kernel arguments from host to device, if in OpenCL you need to do 
`clSetKernelArg`, in SYCL all kernel arguments are captures/fields of 
lambda/functor which is passed to `parallel_for` (See code snippet above, here 
one kernel argument - accessor `A` ). To map to OpenCL setting kernel arguments 
mechanism we added generation of some "kernel wrapper" function inside the 
compiler. "Kernel wrapper" function contains body of SYCL kernel function, 
receives OpenCL like parameters and additionally does some manipulation to 
initialize captured lambda fields with this parameters. In some pseudo code 
"kernel wrapper" looks like this:

  // SYCL kernel is defined in SYCL headers
  __kernel someSYCLKernel(lambda) {
lambda();
  }
  // Kernel wrapper
  __kernel wrapper(global int* a) {
lambda; // Actually lambda declaration doesn't have a name in AST
// Let lambda has one captured field - accessor A. We need to init it with 
global pointer from arguments:
lambda.A.__init(a);
// Body of SYCL kernel from SYCL headers:
{
  lambda();
}
  }

And actually kernel wrapper is presented in result module and passed to OpenCL 
backend.
As I said, kernel wrapper is generated by the compiler inside the Sema and 
OpenCLKernel attribute manually added to it, no matter which attribute was 
added to "SYCL kernel" from SYCL headers.
So, while we are generating this wrapper I see only one profit to use OpenCL 
kernel attribute in SYCL kernels - don't add new attribute to clang (but we 
need to add `__kernel` keyword to SYCL).
I thought about idea - don't generate kernel wrapper but looks like it will not 
work with OpenCL since we can't pass OpenCL `cl_mem` arguments inside any 
structures (including accessors and lambdas) to the kernel.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 complimented with simple compiler 
directives to instruct the compiler about the parallelization.  Hence exactly 
the same program can be  used on sequential or parallel architectures. I can't 
imagine however anyone would use SYCL program on a non-parallel architecture? 
And this is where it is fundamentally different concept to me than C++ that has 
very different execution model (using very explicit language constructs for 
parallelism btw!).

To me SYCL dictates how program is to be written with explicit parallelism 
constructs using a special language. The fact that the language doesn't use 
different syntax from standard C++ at the moment doesn't mean that it's not 
there at least implicitly. If you would be able to just reuse C++ it would be 
perfectly a library style language but since you need language extensions to 
the compiler it isn't just a pure C++ library to me.

> But that means also there is not the same concept of explicit kernel like in 
> OpenCL or CUDA. In OpenCL or CUDA, when there is a function with a specific 
> attribute, you know it is a kernel and you can compile as such.

I am very confused, because if you don't need an explicit kernel construct why 
are you adding it here at all? The fact that you don't provide the 
documentation for it in the spec but yet add it as an explicit attribute in the 
language to allow implementing the feature does show that it is actually 
explicitly required. It is just well hidden behind the C++ library syntax that 
however requires activating features that aren't part of ISO C++. Perhaps I am 
still missing something but I am just worried that we are going to end up with 
a language that pretends to be a C++ library. I certainly see that CUDA or 
OpenCL could just add a layer of C++ libraries on top of their language 
extensions to provide the same functionality. So I still feel SYCL is closer to 
CUDA than to pure C++.

> In SYCL or OpenMP you need an outliner that will estimate what should be 
> executed as an heterogeneous kernel, split the code between the host side and 
> the device side, add some glue/stub to implement an RPC between the host and 
> the device, manage potentially some allocation/memory transfers, etc.

But in SYCL this is requested explicitly in the source code using language 
constructs, isn't it?

> This is quite more complex than compiling OpenCL, CUDA or other graphics 
> shader languages.

I think CUDA still does fair bit of similar logic what you describe above 
though.

> This is also why, while SYCL is technically pure standard C++, you need some 
> specific compiler magic to do the code massaging to have everything working 
> well between a host and some devices.

This patch is actually extending pure standard C++ to make it less pure. There 
is nothing magic about it.

> The attribute we discuss here is just an implementation detail to help the 
> coordination between the compiler and the SYCL frontend classes to mark some 
> area to outline, without relying to do some precise pattern matching, 
> allowing more flexibility in the runtime without changing the compiler every 
> time. So while it defines a zone to be outlined as a kernel, it is not really 
> a kernel in the sense of OpenCL.

Can you give some concrete examples of why device outlined functions can't be 
an OpenCL kernel or functions? What functionality (apart from kernel templates) 
wouldn't be applicable?

> In triSYCL I made some completely different choices, using late outlining in 
> LLVM and detecting some specific functions such as 
> `cl::sycl::detail::instantiate_kernel()` that defines some stuff 
> I want to outline 
> https://github.com/triSYCL/triSYCL/blob/master/doc/architecture.rst#low-level-view-of-the-device-compiler-workflow
>  For me an attribute was not an option because I wanted to change Clang as 
> little as possible.

Why not to continue this approach? What limitation does it have? Is it 
something that demonstrates that the language extension is the real solution to 
this?

> But at the end, I think it is quite more brittle than doing early outlining 
> in Clang as discussed here, which also requires quite more knowledge of Clang 
> than I have. :-)
> 
> So at the end, I think we should use a different keyword from OpenCL or CUDA 
> because the semantics is different.

Overall, I see that this work is now going into a very different direction from 
what was written in the original RFC.
https://lists.llvm.org/pipermail/cfe-dev/2019-January/060811.html

It was suggested that SYCL builds on top of OpenCL and therefore most of 
functionality can be reused. May be the best approach is to restart the RFC 
making the new intent and the overall conce

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

[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 familiar enough with both. Because apart from the 
> public announcements I can't see what has been changed in SYCL that would 
> disallow to use OpenCL mode. It would be a valuable input to determine the 
> implementation choices.

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.
But that means also there is not the same concept of explicit kernel like in 
OpenCL or CUDA. In OpenCL or CUDA, when there is a function with a specific 
attribute, you know it is a kernel and you can compile as such.

In SYCL or OpenMP you need an outliner that will estimate what should be 
executed as an heterogeneous kernel, split the code between the host side and 
the device side, add some glue/stub to implement an RPC between the host and 
the device, manage potentially some allocation/memory transfers, etc. This is 
quite more complex than compiling OpenCL, CUDA or other graphics shader 
languages. This is also why, while SYCL is technically pure standard C++, you 
need some specific compiler magic to do the code massaging to have everything 
working well between a host and some devices.

The attribute we discuss here is just an implementation detail to help the 
coordination between the compiler and the SYCL frontend classes to mark some 
area to outline, without relying to do some precise pattern matching, allowing 
more flexibility in the runtime without changing the compiler every time. So 
while it defines a zone to be outlined as a kernel, it is not really a kernel 
in the sense of OpenCL.

In triSYCL I made some completely different choices, using late outlining in 
LLVM and detecting some specific functions such as 
`cl::sycl::detail::instantiate_kernel()` that defines some stuff I 
want to outline 
https://github.com/triSYCL/triSYCL/blob/master/doc/architecture.rst#low-level-view-of-the-device-compiler-workflow
For me an attribute was not an option because I wanted to change Clang as 
little as possible. But at the end, I think it is quite more brittle than doing 
early outlining in Clang as discussed here, which also requires quite more 
knowledge of Clang than I have. :-)

So at the end, I think we should use a different keyword from OpenCL or CUDA 
because the semantics is different.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 headers to mark the functions to be outlined 
to the device.
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L267
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L295
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L308
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L325

> My understanding of SYCL kernel is that it mainly matches OpenCL kernel 
> functionality because the original intent of SYCL was to provide single 
> source functionality on top of OpenCL.

Yes, this was the idea, when OpenCL was announced in November 2008, to build 
some higher-level programming models on top of it.
Now the SYCL standard is evolving to something more general to bring 
heterogeneous computing to modern C++.
So we could reuse in the same way some attributes from OpenMP 5 or CUDA if the 
Clang/LLVM community thinks it is better.

> But I am not an expert in SYCL to confirm that.

I am pretty sure that the SYCL standard committee would love some active 
participation from ARM. ;-)

> 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 restrictions, address spaces, vectors, special types, etc.

That would make definitely sense when we target OpenCL.

> I still see no point in polluting our code base with extra code that just 
> does the same thing. It will save us a lot of time to just work cooperatively 
> on the same problem and even improve readability of the code. But of course 
> this can only be done if there is no need to diverge the implementation 
> significantly.

Yes. Probably that even when the target is not OpenCL, the general principles 
remain similar. Probably the same for CUDA & OpenMP 5 too...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 restrictions, address spaces, vectors, special 
>> types, etc.
> 
> That would make definitely sense when we target OpenCL.
> 
>> I still see no point in polluting our code base with extra code that just 
>> does the same thing. It will save us a lot of time to just work 
>> cooperatively on the same problem and even improve readability of the code. 
>> But of course this can only be done if there is no need to diverge the 
>> implementation significantly.
> 
> Yes. Probably that even when the target is not OpenCL, the general principles 
> remain similar. Probably the same for CUDA & OpenMP 5 too...

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 familiar enough with both. Because apart from the 
public announcements I can't see what has been changed in SYCL that would 
disallow to use OpenCL mode. It would be a valuable input to determine the 
implementation choices.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 preceding its use. When the function is being 
> > parsed it's not known yet whether it will be called from the device or not, 
> > so it won't be possible to set the language mode correctly and hence 
> > provide the right diagnostics. So is the plan to launch a separate parsing 
> > phase then just to extract the call graph and annotate the device functions?
>
>
> I'm not an expert in clang terminology but I will try briefly explain our 
> current implementation approach. 
>  In SYCL all kernel functions should be template functions so these functions 
> have a deferred instantiation. If we found that we instantiated a sycl kernel 
> function - we add it to a special array with sycl device functions (you can 
> see the corresponding code here 
> 
>  and here 
> ,
>  actually `AddSyclKernel` adds declaration to the special array inside the 
> Sema).  After performing
>  pending instantiations we run a recursive AST visitor for each SYCL kernel 
> to mark all device functions and add them to a special array with SYCL device 
> functions (here 
> 
>  we start traverse AST from `MarkDevice` function, code of `MarkDevice` is 
> here 
> ).
>  To get a correct set of SYCL device functions in produced module we added a 
> check for all declarations inside the CodeGen on  `sycl_device` attribute 
> existence - so it will ignore declarations without `sycl_device` attribute if 
> we are compiling for SYCL device (code is here 
> ).
>  But with this check it's possible situation when function was parsed and 
> ignored by the CodeGen before we added `sycl_device` attribute to it so we 
> added yet another parsing action inside the `clang::ParseAST` to generate 
> code for all SYCL device functions from the special array (code is here 
> ).


Thanks for explanation! I would need to spend a bit more time to go through the 
pointers you have provided. Btw just to understand whether the use case with 
externally defined device side functions is covered too? I.e. can you have 
something like this:

  extern void foo();
  [clang::sycl_kernel]] void bar() {
foo();
  }

When `foo` is defined in a separate module that doesn't call it on a device side

  void foo() {
dosomething();
  }

would compiler still be able to detect that this is a device function?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 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 we can simply add an extra 
> > > > check for SYCL compilation mode in this template handling case. The 
> > > > overall interaction between OpenCL and SYCL implementation is still a 
> > > > very big unknown to me so it's not very easy to judge about the 
> > > > implementations details...
> > >
> > > Of course, if nothing prevents us to re-use OpenCL kernel attribute for 
> > > SYCL I assume it would be good idea. 
> > >  But I'm thinking about the situation with 
> > > https://reviews.llvm.org/D60454 . If we re-use OpenCL kernel attributes - 
> > > we affected by OpenCL-related changes and OpenCL-related changes 
> > > shouldn't violate SYCL semantics. Will it be usable for SYCL/OpenCL clang 
> > > developers? @bader , what do you think about it?
> >
> >
> > I also think it's worth trying. We should be able to cover "SYCL semantics" 
> > with LIT test to avoid regressions by OpenCL related changes. E.g. add a 
> > test case checking that -fsycl-is-device option disables restriction on 
> > applying `__kernel` to template functions.
> >  I want to confirm that everyone is okay to enable `__kernel` keyword for 
> > SYCL extension and cover SYCL use cases with additional regression tests. 
> > IIRC, on yesterday call, @keryell, said that having SYCL specific 
> > attributes useful for separation of concerns.
>
>
> I'm not comfortable with that decision unless the attribute semantics are 
> sufficiently related to justify it. If we're just going to have a lot of 
> `KernelAttr->isSYCL()` vs `KernelAttr->isOpenCL()` accessor calls, it may 
> make more sense to use separate semantic attributes (even if they share 
> spellings), though then I'd be curious how a user combines OpenCL and SYCL 
> attributes.


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? My 
understanding of SYCL kernel is that it mainly matches OpenCL kernel 
functionality because the original intent of SYCL was to provide single source 
functionality on top of OpenCL. But I am not an expert in SYCL to confirm that. 
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 restrictions, address spaces, vectors, special types, etc. I still see 
no point in polluting our code base with extra code that just does the same 
thing. It will save us a lot of time to just work cooperatively on the same 
problem and even improve readability of the code. But of course this can only 
be done if there is no need to diverge the implementation significantly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 which doesn't really seem like an ideal design 
> > > choice. Is this the only difference then we can simply add an extra check 
> > > for SYCL compilation mode in this template handling case. The overall 
> > > interaction between OpenCL and SYCL implementation is still a very big 
> > > unknown to me so it's not very easy to judge about the implementations 
> > > details...
> >
> > Of course, if nothing prevents us to re-use OpenCL kernel attribute for 
> > SYCL I assume it would be good idea. 
> >  But I'm thinking about the situation with https://reviews.llvm.org/D60454 
> > . If we re-use OpenCL kernel attributes - we affected by OpenCL-related 
> > changes and OpenCL-related changes shouldn't violate SYCL semantics. Will 
> > it be usable for SYCL/OpenCL clang developers? @bader , what do you think 
> > about it?
>
>
> I also think it's worth trying. We should be able to cover "SYCL semantics" 
> with LIT test to avoid regressions by OpenCL related changes. E.g. add a test 
> case checking that -fsycl-is-device option disables restriction on applying 
> `__kernel` to template functions.
>  I want to confirm that everyone is okay to enable `__kernel` keyword for 
> SYCL extension and cover SYCL use cases with additional regression tests. 
> IIRC, on yesterday call, @keryell, said that having SYCL specific attributes 
> useful for separation of concerns.


I'm not comfortable with that decision unless the attribute semantics are 
sufficiently related to justify it. If we're just going to have a lot of 
`KernelAttr->isSYCL()` vs `KernelAttr->isOpenCL()` accessor calls, it may make 
more sense to use separate semantic attributes (even if they share spellings), 
though then I'd be curious how a user combines OpenCL and SYCL attributes.

In D60455#1468779 , @bader wrote:

> @aaron.ballman sorry for confusion.
>  SYCL specification doesn't require user to annotate "device functions" with 
> an attribute - it says following (from section 6.9.1 SYCL functions and 
> methods linkage, https://www.khronos.org/registry/SYCL/specs/sycl-1.2.1.pdf, 
> page 251):
>
> > The default behavior in SYCL applications is that all the definitions and 
> > declarations of the functions and methods
> >  are available to the SYCL compiler, in the same translation unit. When 
> > this is not the case, all the symbols that
> >  need to be exported to a SYCL library or from a C++ library to a SYCL 
> > application need to be defined using the
> >  macro: SYCL_EXTERNAL.
> >  The SYCL_EXTERNAL macro will only be defined if the implementation 
> > supports offline linking. The macro is
> >  implementation-defined, but the following restrictions apply:
> > 
> > • SYCL_EXTERNAL can only be used on functions;
> >  • the function cannot use raw pointers as parameter or return types. 
> > Explicit pointer classes must be used instead;
> >  • externally defined functions cannot call a 
> > cl::sycl::parallel_for_work_item method;
> >  • externally defined functions cannot be called from a 
> > cl::sycl::parallel_for_work_group scope.
> > 
> > The SYCL linkage mechanism is optional and implementation defined.
>
> The idea I had is that to define `SYCL_EXTERNAL` macro as `sycl_device` 
> attribute.


That seems sensible, but then we're also missing extra semantic checks in this 
patch, such as prohibiting raw pointers, etc. Those should get added along with 
tests.

> BTW, I noticed that `SYCL_EXTERNAL` puts additional requirements 
> `sycl_device` doesn't meet:
> 
>> • SYCL_EXTERNAL can only be used on functions;
> 
> I think our implementation doesn't have such limitations and able to support 
> more use cases.

Does it make sense to deviate from the SYCL spec though?

> Anyway, we can make `sycl_device` attribute implicit for now and return to 
> the implementation of cross translation unit dependencies later.

That might be an easier first-pass for the attribute. I'm not at all opposed to 
giving it a name, I was just trying to keep the threads of discussion straight. 
:-)

In D60455#1468544 , @Fznamznon wrote:

> 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 the compiler differ SYCL device code from host 
> code and ignore functions without device attributes when SYCL device mode is 
> enabled. I've described some possible implementation details in this comment 
> .


Ah, thank you for tha

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

[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 in the clang namespace. I 
> > didn't find path to add two namespaces to attribute (like 
> > [[clang::sycl::device]]) so [[clang::sycl_device]] spelling is added.
> > - Since both attributes have spellings and possible can be used as some 
> > "standard" outlining in Clang/LLVM I added documetation.
> > - Added more test cases.
> > - As @bader mentioned sycl_device can be used to mark functions, which are 
> > called from the different translation units so I added simple handling of 
> > this attribute in Sema.
>
>
> I'm confused -- I thought @bader also said "...SYCL is not supposed to expose 
> any non-standard extensions to a user." -- these attributes are not standards 
> based (WG21 and WG14 have nothing to say about them), so are these attributes 
> considered "non-standard extensions" or not?


@aaron.ballman sorry for confusion.
SYCL specification doesn't require user to annotate "device functions" with an 
attribute - it says following (from section 6.9.1 SYCL functions and methods 
linkage, https://www.khronos.org/registry/SYCL/specs/sycl-1.2.1.pdf, page 251):

> The default behavior in SYCL applications is that all the definitions and 
> declarations of the functions and methods
>  are available to the SYCL compiler, in the same translation unit. When this 
> is not the case, all the symbols that
>  need to be exported to a SYCL library or from a C++ library to a SYCL 
> application need to be defined using the
>  macro: SYCL_EXTERNAL.
>  The SYCL_EXTERNAL macro will only be defined if the implementation supports 
> offline linking. The macro is
>  implementation-defined, but the following restrictions apply:
> 
> • SYCL_EXTERNAL can only be used on functions;
>  • the function cannot use raw pointers as parameter or return types. 
> Explicit pointer classes must be used instead;
>  • externally defined functions cannot call a 
> cl::sycl::parallel_for_work_item method;
>  • externally defined functions cannot be called from a 
> cl::sycl::parallel_for_work_group scope.
> 
> The SYCL linkage mechanism is optional and implementation defined.

The idea I had is that to define `SYCL_EXTERNAL` macro as `sycl_device` 
attribute.

BTW, I noticed that `SYCL_EXTERNAL` puts additional requirements `sycl_device` 
doesn't meet:

> • SYCL_EXTERNAL can only be used on functions;

I think our implementation doesn't have such limitations and able to support 
more use cases.
Anyway, we can make `sycl_device` attribute implicit for now and return to the 
implementation of cross translation unit dependencies later.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 we can simply add an extra check for SYCL 
> > compilation mode in this template handling case. The overall interaction 
> > between OpenCL and SYCL implementation is still a very big unknown to me so 
> > it's not very easy to judge about the implementations details...
>
> Of course, if nothing prevents us to re-use OpenCL kernel attribute for SYCL 
> I assume it would be good idea. 
>  But I'm thinking about the situation with https://reviews.llvm.org/D60454 . 
> If we re-use OpenCL kernel attributes - we affected by OpenCL-related changes 
> and OpenCL-related changes shouldn't violate SYCL semantics. Will it be 
> usable for SYCL/OpenCL clang developers? @bader , what do you think about it?


I also think it's worth trying. We should be able to cover "SYCL semantics" 
with LIT test to avoid regressions by OpenCL related changes. E.g. add a test 
case checking that -fsycl-is-device option disables restriction on applying 
`__kernel` to template functions.
I want to confirm that everyone is okay to enable `__kernel` keyword for SYCL 
extension and cover SYCL use cases with additional regression tests. IIRC, on 
yesterday call, @keryell, said that having SYCL specific attributes useful for 
separation of concerns.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 the compiler differ SYCL device code from host code 
and ignore functions without device attributes when SYCL device mode is 
enabled. I've described some possible implementation details in this comment 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 -x c++ %s

aaron.ballman wrote:
> This comment confuses me -- the file *is* a C++ file and the preceding RUN 
> line treats it as such, doesn't it?
I've tried to say that we compile regular C++ file without SYCL mode enabled. 
Looks like it can confuse since files with SYCL code have cpp extension... I 
will rewrite this comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 description, it sounds like whoever authors `parallel_for` needs 
> > > to do this marking, or it somehow happens automatically?
> > > 
> > > (I'll do another editorial pass once I understand the intended behavior a 
> > > bit better -- I expect there will be a few more wording issues to 
> > > address.)
> > In normal SYCL it happens automatically.
> > In the compiler unit-tests it is done manually to exercise the framework.
> > I am the one who suggested that in some other contexts, it could be used 
> > manually for some special purpose like using some weird hardware, but I do 
> > not want to derail the main review with this.
> > In normal SYCL it happens automatically.
> > In the compiler unit-tests it is done manually to exercise the framework.
> > 
> 
> 
> I think if they are not to be exposed to the user they should have no 
> spelling. There are plenty of other ways to test this. For example AST dump.
> 
> 
> > I am the one who suggested that in some other contexts, it could be used 
> > manually for some special purpose like using some weird hardware, but I do 
> > not want to derail the main review with this.
> 
> If you are suggesting to expose this feature then you are starting some sort 
> of a language extensions and its use should be documented in some way. I am 
> not sure about this but I think we will end up with some sort of a language 
> extension for SYCL anyways because as it stands now it's not aligned with the 
> general concept of C/C++ language design. So perhaps it's not entirely 
> unreasonable to expose this.
Generally the `sycl_device` attribute will be added automatically by the 
compiler. But as @bader mentioned before:
> we might need to use sycl_device attribute to mark functions, which are 
> called from the different translation units, i.e. compiler can't identify it 
> w/o user's help.
> SYCL specification proposes to use special macro as "device function marker", 
> but I guess we can have additional "spellings" in the clang.
I think It would be better to re-use this attribute in implementation of 
"device function marker" macro from SYCL spec than implement additional logic 
in the compiler to handle this macro. So I saved possibility to add this 
attribute in code.






Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 will be called from the device or not, so it won't 
> be possible to set the language mode correctly and hence provide the right 
> diagnostics. So is the plan to launch a separate parsing phase then just to 
> extract the call graph and annotate the device functions?


I'm not an expert in clang terminology but I will try briefly explain our 
current implementation approach. 
In SYCL all kernel functions should be template functions so these functions 
have a deferred instantiation. If we found that we instantiated a sycl kernel 
function - we add it to a special array with sycl device functions (you can see 
the corresponding code here 

 and here 
,
 actually `AddSyclKernel` adds declaration to the special array inside the 
Sema).  After performing
pending instantiations we run a recursive AST visitor for each SYCL kernel to 
mark all device functions and add them to a special array with SYCL device 
functions (here 

 we start traverse AST from `MarkDevice` function, code of `MarkDevice` is here 
).
To get a correct set of SYCL device functions in produced module we added a 
check for all declarations inside the CodeGen on  `sycl_device` attribute 
existence - so it will ignore declarations without `sycl_device` attribute if 
we are compiling for SYCL device (code is here 
).
 But with this check it's possible situation when function was parsed and 
ignored by the CodeGen before we added `sycl_device' attribute to it so we 
added yet another parsing action inside the clang::ParseAST to generate code 
for all SYCL device functions from the special array (code is here 
).

> 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 we can simply add an extra check for SYCL 
> compilation mode in this template handling case. The overall interaction 
> between OpenCL and SYCL implementation is still a very big unknown to me so 
> it's not very easy to judge about the implementations details...

Of course, if nothing prevents us to re-use OpenCL kernel attribute for SYCL I 
assume it would be good idea. 
But I'm thinking about the situation with https://reviews.llvm.org/D60454 . If 
we re-use OpenCL kernel attributes - we affected by OpenCL-related changes and 
OpenCL-related changes shouldn't violate SYCL semantics. Will it be usable for 
SYCL/OpenCL clang developers? @bader , what do you think about it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 whoever authors `parallel_for` needs to do 
> > this marking, or it somehow happens automatically?
> > 
> > (I'll do another editorial pass once I understand the intended behavior a 
> > bit better -- I expect there will be a few more wording issues to address.)
> In normal SYCL it happens automatically.
> In the compiler unit-tests it is done manually to exercise the framework.
> I am the one who suggested that in some other contexts, it could be used 
> manually for some special purpose like using some weird hardware, but I do 
> not want to derail the main review with this.
> In normal SYCL it happens automatically.
> In the compiler unit-tests it is done manually to exercise the framework.
> 


I think if they are not to be exposed to the user they should have no spelling. 
There are plenty of other ways to test this. For example AST dump.


> I am the one who suggested that in some other contexts, it could be used 
> manually for some special purpose like using some weird hardware, but I do 
> not want to derail the main review with this.

If you are suggesting to expose this feature then you are starting some sort of 
a language extensions and its use should be documented in some way. I am not 
sure about this but I think we will end up with some sort of a language 
extension for SYCL anyways because as it stands now it's not aligned with the 
general concept of C/C++ language design. So perhaps it's not entirely 
unreasonable to expose this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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:
> aaron.ballman wrote:
> > keryell wrote:
> > > Fznamznon wrote:
> > > > aaron.ballman wrote:
> > > > > Is there a reason to not also introduce a C++11 and C2x style 
> > > > > spelling in the `clang` namespace? e.g., `[[clang::sycl_device]]`
> > > > I don't think that it makes sense because these attributes not for 
> > > > public consumption. These attributes is needed to separate code which 
> > > > is supposed to be offloaded from regular host code. I think SYCLDevice 
> > > > attribute actually doesn't need a spelling because it will be added 
> > > > only implicitly by compiler.
> > > 
> > > If we go towards this direction, `[[clang::sycl::device]]` or 
> > > `[[clang::sycl::kernel]]` look more compatible with the concept of name 
> > > space.
> > > While not a public interface, if we have a kind of "standard" outlining 
> > > in Clang/LLVM, some people might want to use it in some other contexts 
> > > too.
> > If these are only being added implicitly by the compiler, then they should 
> > not be given any `Spelling`. See `AlignMac68k` for an example.
> I'm still confused -- are these created implicitly or are they spelled out by 
> the user explicitly? Right now, it looks like they're spelled out explicitly, 
> but I was under the impression they are only intended to be created 
> implicitly by the compiler.
> 
> If they are expected to be explicitly specified by the user, the spelling 
> should be using `Clang<>` instead of using `GNU<>`, `C2x<>`, and `CXX11<>` 
> explicitly.
> 
> > If we go towards this direction, [[clang::sycl::device]] or 
> > [[clang::sycl::kernel]] look more compatible with the concept of name space.
> 
> Attribute namespaces do not work that way. There is the vendor namespace and 
> then the attribute name.
> Attribute namespaces do not work that way. There is the vendor namespace and 
> then the attribute name.

After diving into "9.11.1 Attribute syntax and semantics [dcl.attr.grammar]" of 
the latest C++ draft standard, it looks you are right... There is only 1 level 
of `::` allowed. :-(



Comment at: clang/include/clang/Basic/AttrDocs.td:286
+help.
+  }];
+}

aaron.ballman wrote:
> I'm still not entirely certain how I would know what to mark and how. From 
> the description, it sounds like whoever authors `parallel_for` needs to do 
> this marking, or it somehow happens automatically?
> 
> (I'll do another editorial pass once I understand the intended behavior a bit 
> better -- I expect there will be a few more wording issues to address.)
In normal SYCL it happens automatically.
In the compiler unit-tests it is done manually to exercise the framework.
I am the one who suggested that in some other contexts, it could be used 
manually for some special purpose like using some weird hardware, but I do not 
want to derail the main review with this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 style spelling in the clang namespace. I didn't 
> find path to add two namespaces to attribute (like [[clang::sycl::device]]) 
> so [[clang::sycl_device]] spelling is added.
> - Since both attributes have spellings and possible can be used as some 
> "standard" outlining in Clang/LLVM I added documetation.
> - Added more test cases.
> - As @bader mentioned sycl_device can be used to mark functions, which are 
> called from the different translation units so I added simple handling of 
> this attribute in Sema.


I'm confused -- I thought @bader also said "...SYCL is not supposed to expose 
any non-standard extensions to a user." -- these attributes are not standards 
based (WG21 and WG14 have nothing to say about them), so are these attributes 
considered "non-standard extensions" or not?

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?




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:
> keryell wrote:
> > Fznamznon wrote:
> > > aaron.ballman wrote:
> > > > Is there a reason to not also introduce a C++11 and C2x style spelling 
> > > > in the `clang` namespace? e.g., `[[clang::sycl_device]]`
> > > I don't think that it makes sense because these attributes not for public 
> > > consumption. These attributes is needed to separate code which is 
> > > supposed to be offloaded from regular host code. I think SYCLDevice 
> > > attribute actually doesn't need a spelling because it will be added only 
> > > implicitly by compiler.
> > 
> > If we go towards this direction, `[[clang::sycl::device]]` or 
> > `[[clang::sycl::kernel]]` look more compatible with the concept of name 
> > space.
> > While not a public interface, if we have a kind of "standard" outlining in 
> > Clang/LLVM, some people might want to use it in some other contexts too.
> If these are only being added implicitly by the compiler, then they should 
> not be given any `Spelling`. See `AlignMac68k` for an example.
I'm still confused -- are these created implicitly or are they spelled out by 
the user explicitly? Right now, it looks like they're spelled out explicitly, 
but I was under the impression they are only intended to be created implicitly 
by the compiler.

If they are expected to be explicitly specified by the user, the spelling 
should be using `Clang<>` instead of using `GNU<>`, `C2x<>`, and `CXX11<>` 
explicitly.

> If we go towards this direction, [[clang::sycl::device]] or 
> [[clang::sycl::kernel]] look more compatible with the concept of name space.

Attribute namespaces do not work that way. There is the vendor namespace and 
then the attribute name.



Comment at: clang/include/clang/Basic/AttrDocs.td:259
+  let Content = [{
+The sycl_device attribute specifies function which is supposed to be compiled
+for the device and cannot be directly called by the host. Here is code example

specifies function which is -> specifies that a function is

Also, please put backticks around `sycl_device`.



Comment at: clang/include/clang/Basic/AttrDocs.td:260
+The sycl_device attribute specifies function which is supposed to be compiled
+for the device and cannot be directly called by the host. Here is code example
+of the SYCL program, which demonstrates the need for this attribute:

is code example -> is a code example



Comment at: clang/include/clang/Basic/AttrDocs.td:261
+for the device and cannot be directly called by the host. Here is code example
+of the SYCL program, which demonstrates the need for this attribute:
+

the SYCL program, which -> a SYLC program that

(Note, I also removed the comma.)



Comment at: clang/include/clang/Basic/AttrDocs.td:277
+
+Code is passed to parallel_for is called "kernel function" and defines some
+entry point to device code i.e. will be called by host in run time. Compiler

Code -> Do you mean the lambda? If so, perhaps "The lambda that is passed to 
the `parallel_for`" (add backticks around parallel_for too, please).

is called "kernel function" -> is called a "kernel function"



Comment at: clang/include/clang/Basic/AttrDocs.td:286
+help.
+  }];
+}

I'm still not entirely certain how I would know what to mark and how. From the 
description, it sounds like whoever authors `parallel_for` needs to do this 
marking, or it somehow happens automatically?

(I'll do another edi

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

[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 symbols accessible from kernel functions and 
> add them to the "device part" of the code.
> 
> Here is a link to the code in the SYCL runtime using `sycl_kernel` attribute: 
> https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L267
> 
> I'm quite sure something similar should happen for other "single source" 
> programming models like OpenMP/CUDA, except these attributes are exposed to 
> the user and there is a specific requirement on attributes/pragma/keyword 
> names.

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 will be called from the device or not, so it won't be 
possible to set the language mode correctly and hence provide the right 
diagnostics. So is the plan to launch a separate parsing phase then just to 
extract the call graph and annotate the device functions?

> NOTE2: @Anastasia, https://reviews.llvm.org/D60454 makes impossible to 
> replace `sycl_kernel` attribute with `__kernel` attribute. I mean we still 
> can enable it for SYCL extension, but we will need SYCL specific 
> customization in this case as we apply `kernel` attribute to a template 
> function.

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 we can simply add an extra check for SYCL compilation 
mode in this template handling case. The overall interaction between OpenCL and 
SYCL implementation is still a very big unknown to me so it's not very easy to 
judge about the implementations details...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp
  clang/test/SemaSYCL/device-attributes.cpp

Index: clang/test/SemaSYCL/device-attributes.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/device-attributes.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only -fsycl-is-device -verify %s
+
+[[clang::sycl_device]] int gv = 0;
+__attribute((sycl_device)) int gv1 = 0;
+[[clang::sycl_kernel]] int gv2 = 0; // expected-warning {{'sycl_kernel' attribute only applies to functions}}
+__attribute((sycl_kernel)) int gv3 = 0; // expected-warning {{'sycl_kernel' attribute only applies to functions}}
+
+__attribute((sycl_kernel)) __attribute((sycl_device)) void foo();
+[[clang::sycl_kernel]] [[clang::sycl_device]] void foo1();
+
+__attribute((sycl_kernel(1))) void foo(); // expected-error {{'sycl_kernel' attribute takes no arguments}}
+__attribute((sycl_device(1))) void foo1(); // expected-error {{'sycl_device' attribute takes no arguments}}
+[[clang::sycl_kernel(1)]] void foo2(); // expected-error {{'sycl_kernel' attribute takes no arguments}}
+[[clang::sycl_device(1)]] void foo3(); // expected-error {{'sycl_device' attribute takes no arguments}}
Index: clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp
@@ -0,0 +1,18 @@
+// 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 -x c++ %s
+
+#if not defined(__SYCL_DEVICE_ONLY__)
+// expected-warning@+8 {{'sycl_kernel' attribute ignored}}
+// expected-warning@+8 {{'sycl_device' attribute ignored}}
+// expected-warning@+8 {{'sycl_kernel' attribute ignored}}
+// expected-warning@+8 {{'sycl_device' attribute ignored}}
+#else
+// expected-no-diagnostics
+#endif
+
+__attribute((sycl_kernel)) void foo();
+__attribute((sycl_device)) void foo1();
+[[clang::sycl_kernel]] void foo2();
+[[clang::sycl_device]] void foo3();
+
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -124,6 +124,8 @@
 // CHECK-NEXT: ReturnTypestate (SubjectMatchRule_function, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ReturnsNonNull (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)
+// CHECK-NEXT: SYCLDevice (SubjectMatchRule_function, SubjectMatchRule_variable)
+// CHECK-NEXT: SYCLKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ScopedLockable (SubjectMatchRule_record)
 // CHECK-NEXT: Section (SubjectMatchRule_function, SubjectMatchRule_variable_is_global, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
 // CHECK-NEXT: SetTypestate (SubjectMatchRule_function_is_member)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6755,6 +6755,12 @@
   case ParsedAttr::AT_Flatten:
 handleSimpleAttribute(S, D, AL);
 break;
+  case ParsedAttr::AT_SYCLDevice:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_SYCLKernel:
+handleSimpleAttribute(S, D, AL);
+break;
   case ParsedAttr::AT_Format:
 handleFormatAttr(S, D, AL);
 break;
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -253,6 +253,50 @@
   }];
 }
 
+def SYCLDeviceDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The sycl_device attribute specifies function which is supposed to be compiled
+for the device and cannot be directly called by the host. Here is code example
+of the SYCL program, which demonstrates the need for this attribute:
+
+.. code-block:: c++
+
+  int foo(int x) { return ++x; }
+
+  using namespace cl::sycl;
+  queue Q;
+  buffer a(range<1>{1024});
+  Q.submit([&](handler& cgh) {
+auto A = a.get_access(cgh);
+cgh.parallel_for(range<1>{1024}, [=](id<1> index) {
+  A[index] = index[0] * 2 + index[1] + foo(42);
+});
+  }
+
+Code is passed to parallel_for is called "kernel function" and defines some
+entry point to device code 

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

[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 -verify -x c++ %s
+

No need to pass "EXPECT_WARNINGS" define.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp
  clang/test/SemaSYCL/device-attributes.cpp

Index: clang/test/SemaSYCL/device-attributes.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/device-attributes.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only -fsycl-is-device -verify %s
+
+[[clang::sycl_device]] int gv = 0;
+__attribute((sycl_device)) int gv1 = 0;
+[[clang::sycl_kernel]] int gv2 = 0; // expected-warning {{'sycl_kernel' attribute only applies to functions}}
+__attribute((sycl_kernel)) int gv3 = 0; // expected-warning {{'sycl_kernel' attribute only applies to functions}}
+
+__attribute((sycl_kernel)) __attribute((sycl_device)) void foo();
+[[clang::sycl_kernel]] [[clang::sycl_device]] void foo1();
+
+__attribute((sycl_kernel(1))) void foo(); // expected-error {{'sycl_kernel' attribute takes no arguments}}
+__attribute((sycl_device(1))) void foo1(); // expected-error {{'sycl_device' attribute takes no arguments}}
+[[clang::sycl_kernel(1)]] void foo2(); // expected-error {{'sycl_kernel' attribute takes no arguments}}
+[[clang::sycl_device(1)]] void foo3(); // expected-error {{'sycl_device' attribute takes no arguments}}
Index: clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp
@@ -0,0 +1,18 @@
+// 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 -DEXPECT_WARNINGS -fsyntax-only -verify -x c++ %s
+
+#if not defined(__SYCL_DEVICE_ONLY__)
+// expected-warning@+8 {{'sycl_kernel' attribute ignored}}
+// expected-warning@+8 {{'sycl_device' attribute ignored}}
+// expected-warning@+8 {{'sycl_kernel' attribute ignored}}
+// expected-warning@+8 {{'sycl_device' attribute ignored}}
+#else
+// expected-no-diagnostics
+#endif
+
+__attribute((sycl_kernel)) void foo();
+__attribute((sycl_device)) void foo1();
+[[clang::sycl_kernel]] void foo2();
+[[clang::sycl_device]] void foo3();
+
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -124,6 +124,8 @@
 // CHECK-NEXT: ReturnTypestate (SubjectMatchRule_function, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ReturnsNonNull (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)
+// CHECK-NEXT: SYCLDevice (SubjectMatchRule_function, SubjectMatchRule_variable)
+// CHECK-NEXT: SYCLKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ScopedLockable (SubjectMatchRule_record)
 // CHECK-NEXT: Section (SubjectMatchRule_function, SubjectMatchRule_variable_is_global, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
 // CHECK-NEXT: SetTypestate (SubjectMatchRule_function_is_member)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6755,6 +6755,12 @@
   case ParsedAttr::AT_Flatten:
 handleSimpleAttribute(S, D, AL);
 break;
+  case ParsedAttr::AT_SYCLDevice:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_SYCLKernel:
+handleSimpleAttribute(S, D, AL);
+break;
   case ParsedAttr::AT_Format:
 handleFormatAttr(S, D, AL);
 break;
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -253,6 +253,50 @@
   }];
 }
 
+def SYCLDeviceDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The sycl_device attribute specifies function which is supposed to be compiled
+for the device and cannot be directly called by the host. Here is code example
+of the SYCL program, which demonstrates the need for this attribute:
+
+.. code-block:: c++
+
+  int foo(int x) { return ++x; }
+
+  using namespace cl::sycl;
+  queue Q;
+  buffer a(range<1>{1024});
+  Q.submit([&](handler& cgh) {
+auto A = a.get_access(cgh);
+cgh.parallel_for(range<1>{1024}, [=](id<1> index) {
+  A[index] = index[0] * 2 + index[1] + foo(42);
+});
+  }
+
+Code is passed to parallel_for is called "kernel function" and defines some
+entry poi

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

[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:
> bader wrote:
> > This duplicates clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp.
> > Maybe it make sense to test case when both attributes are applied to the 
> > same function.
> From current documentation: sycl_kernel can be applied to function which can 
> be directly called by the host and will be compiled for device, sycl_device 
> applies to device functions which **cannot** be directly called by the 
> host... 
> I think if we want add this test case we need clarify how this case will be 
> handled by the compiler.
> From current documentation: sycl_kernel can be applied to function which can 
> be directly called by the host and will be compiled for device, sycl_device 
> applies to device functions which cannot be directly called by the host... 
> I think if we want add this test case we need clarify how this case will be 
> handled by the compiler.

I would expect `sycl_kernel` to supersede `sycl_device` attribute. Basically 
`sycl_kernel` include functionality of `sycl_device` attribute and provides 
additional host accessibility.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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:
> This duplicates clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp.
> Maybe it make sense to test case when both attributes are applied to the same 
> function.
From current documentation: sycl_kernel can be applied to function which can be 
directly called by the host and will be compiled for device, sycl_device 
applies to device functions which **cannot** be directly called by the host... 
I think if we want add this test case we need clarify how this case will be 
handled by the compiler.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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__` define instead.



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();

This duplicates clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp.
Maybe it make sense to test case when both attributes are applied to the same 
function.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 
[[clang::sycl_device]] spelling is added.
- Since both attributes have spellings and possible can be used as some 
"standard" outlining in Clang/LLVM I added documetation.
- Added more test cases.
- As @bader mentioned sycl_device can be used to mark functions, which are 
called from the different translation units so I added simple handling of this 
attribute in Sema.


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/include/clang/Basic/AttrDocs.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp
  clang/test/SemaSYCL/device-attrubutes.cpp

Index: clang/test/SemaSYCL/device-attrubutes.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/device-attrubutes.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -fsycl-is-device -verify %s
+
+[[clang::sycl_device]] int gv = 0;
+__attribute((sycl_device)) int gv1 = 0;
+[[clang::sycl_kernel]] int gv2 = 0; // expected-warning {{'sycl_kernel' attribute only applies to functions}}
+__attribute((sycl_kernel)) int gv3 = 0; // expected-warning {{'sycl_kernel' attribute only applies to functions}}
+
+__attribute((sycl_kernel)) void foo();
+__attribute((sycl_device)) void foo1();
+[[clang::sycl_kernel]] void foo2();
+[[clang::sycl_device]] void foo3();
+
+__attribute((sycl_kernel(1))) void foo(); // expected-error {{'sycl_kernel' attribute takes no arguments}}
+__attribute((sycl_device(1))) void foo1(); // expected-error {{'sycl_device' attribute takes no arguments}}
+[[clang::sycl_kernel(1)]] void foo2(); // expected-error {{'sycl_kernel' attribute takes no arguments}}
+[[clang::sycl_device(1)]] void foo3(); // expected-error {{'sycl_device' attribute takes no arguments}}
Index: clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp
@@ -0,0 +1,18 @@
+// 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 -DEXPECT_WARNINGS -fsyntax-only -verify -x c++ %s
+
+#if defined(EXPECT_WARNINGS)
+// expected-warning@+8 {{'sycl_kernel' attribute ignored}}
+// expected-warning@+8 {{'sycl_device' attribute ignored}}
+// expected-warning@+8 {{'sycl_kernel' attribute ignored}}
+// expected-warning@+8 {{'sycl_device' attribute ignored}}
+#else
+// expected-no-diagnostics
+#endif
+
+__attribute((sycl_kernel)) void foo();
+__attribute((sycl_device)) void foo1();
+[[clang::sycl_kernel]] void foo2();
+[[clang::sycl_device]] void foo3();
+
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -124,6 +124,8 @@
 // CHECK-NEXT: ReturnTypestate (SubjectMatchRule_function, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ReturnsNonNull (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)
+// CHECK-NEXT: SYCLDevice (SubjectMatchRule_function, SubjectMatchRule_variable)
+// CHECK-NEXT: SYCLKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ScopedLockable (SubjectMatchRule_record)
 // CHECK-NEXT: Section (SubjectMatchRule_function, SubjectMatchRule_variable_is_global, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
 // CHECK-NEXT: SetTypestate (SubjectMatchRule_function_is_member)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6755,6 +6755,12 @@
   case ParsedAttr::AT_Flatten:
 handleSimpleAttribute(S, D, AL);
 break;
+  case ParsedAttr::AT_SYCLDevice:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_SYCLKernel:
+handleSimpleAttribute(S, D, AL);
+break;
   case ParsedAttr::AT_Format:
 handleFormatAttr(S, D, AL);
 break;
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -253,6 +253,48 @@
   }];
 }
 
+def SYCLDeviceDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The sycl_device attribute specifies function which is supposed to be 

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

[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:
> > aaron.ballman wrote:
> > > Is there a reason to not also introduce a C++11 and C2x style spelling in 
> > > the `clang` namespace? e.g., `[[clang::sycl_device]]`
> > I don't think that it makes sense because these attributes not for public 
> > consumption. These attributes is needed to separate code which is supposed 
> > to be offloaded from regular host code. I think SYCLDevice attribute 
> > actually doesn't need a spelling because it will be added only implicitly 
> > by compiler.
> 
> If we go towards this direction, `[[clang::sycl::device]]` or 
> `[[clang::sycl::kernel]]` look more compatible with the concept of name space.
> While not a public interface, if we have a kind of "standard" outlining in 
> Clang/LLVM, some people might want to use it in some other contexts too.
If these are only being added implicitly by the compiler, then they should not 
be given any `Spelling`. See `AlignMac68k` for an example.



Comment at: clang/include/clang/Basic/Attr.td:1003
+  let LangOpts = [SYCL];
+  let Documentation = [Undocumented];
+}

keryell wrote:
> Fznamznon wrote:
> > aaron.ballman wrote:
> > > No new, undocumented attributes, please.
> > As I said, these attributes are not for public consumption. Should I add 
> > documentation in this case too?
> Yes, documentation and comments are always appreciated.
If the attribute is only ever added implicitly and the user cannot spell it in 
any way, then I think it's reasonable to elide the documentation. It's an 
implementation detail at that point.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 TU by design.




Comment at: clang/include/clang/Basic/Attr.td:1000
+def SYCLDevice : InheritableAttr {
+  let Spellings = [GNU<"sycl_device">];
+  let Subjects = SubjectList<[Function, Var]>;

Fznamznon wrote:
> aaron.ballman wrote:
> > Is there a reason to not also introduce a C++11 and C2x style spelling in 
> > the `clang` namespace? e.g., `[[clang::sycl_device]]`
> I don't think that it makes sense because these attributes not for public 
> consumption. These attributes is needed to separate code which is supposed to 
> be offloaded from regular host code. I think SYCLDevice attribute actually 
> doesn't need a spelling because it will be added only implicitly by compiler.

If we go towards this direction, `[[clang::sycl::device]]` or 
`[[clang::sycl::kernel]]` look more compatible with the concept of name space.
While not a public interface, if we have a kind of "standard" outlining in 
Clang/LLVM, some people might want to use it in some other contexts too.



Comment at: clang/include/clang/Basic/Attr.td:1003
+  let LangOpts = [SYCL];
+  let Documentation = [Undocumented];
+}

Fznamznon wrote:
> aaron.ballman wrote:
> > No new, undocumented attributes, please.
> As I said, these attributes are not for public consumption. Should I add 
> documentation in this case too?
Yes, documentation and comments are always appreciated.



Comment at: clang/include/clang/Basic/Attr.td:1010
+  let LangOpts = [SYCL];
+  let Documentation = [Undocumented];
+}

Cf supra.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 demonstrate the need for these 
attributes:

  C++
  int foo(int x) { return ++x; }
  int bar(int x) { throw std::exception("CPU code only!"); }
  …
  using namespace cl::sycl;
  queue Q;
  buffer a(range<1>{1024});
  Q.submit([&](handler& cgh) {
auto A = a.get_access(cgh);
cgh.parallel_for(range<1>{1024}, [=](id<1> index) {
  A[index] = index[0] * 2 + index[1] + foo(42);
});
  }
  ...

SYCL compiler need to compile lambda function passed to 
`cl::sycl::handler::parallel_for` method and function `foo` called from this 
lambda function.

NOTE: compiler must ignore `bar` function when we "device" part of the single 
source code.

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 symbols accessible from kernel functions and add 
them to the "device part" of the code.

Here is a link to the code in the SYCL runtime using `sycl_kernel` attribute: 
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L267

I'm quite sure something similar should happen for other "single source" 
programming models like OpenMP/CUDA, except these attributes are exposed to the 
user and there is a specific requirement on attributes/pragma/keyword names.

What we are looking for is whether we should add SYCL specific code or we can 
come up with something more generic to avoid logic/code duplication with 
already existing functionality.

BTW: Mariya, I think we might need to use `sycl_device` attribute to mark 
functions, which are called from the different translation units, i.e. compiler 
can't identify it w/o user's help.
SYCL specification proposes to use special macro as "device function marker", 
but I guess we can have additional "spellings" in the clang.

NOTE2: @Anastasia, https://reviews.llvm.org/D60454 makes impossible to replace 
`sycl_kernel` attribute with `__kernel` attribute. I mean we still can enable 
it for SYCL extension, but we will need SYCL specific customization in this 
case as we apply `kernel` attribute to a template function.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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?
>
> I have raised a couple of related questions on the earlier RFC btw but there 
> hasn't been any follow up since then I believe.


Good point. But in SYCL actually two types of functions that can be compiled 
for device: SYCL kernels and device functions. SYCL kernels are analog for 
OpenCL kernels, these are some functions that can be called from host. SYCL 
kernels should also have OpenCL kernel attribute to be OpenCL kernels. But if 
SYCL kernel calls any function - this function is compiled for device and 
called a “device function”. We need to detect it in compiler. And "device 
function" cannot be called from host and cannot be OpenCL kernel. I think there 
is no attribute for SYCL "device functions" in OpenCL because there is no 
"device functions".
So, I think I can reuse OpenCL kernel attribute (possible with __kernel 
keyword) for SYCL kernels but I still need SYCLDevice attribute to mark "device 
functions".
What do you think about it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 attribute?

I have raised a couple of related questions on the earlier RFC btw but there 
hasn't been any follow up since then I believe.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 introduce a C++11 and C2x style spelling in the 
> `clang` namespace? e.g., `[[clang::sycl_device]]`
I don't think that it makes sense because these attributes not for public 
consumption. These attributes is needed to separate code which is supposed to 
be offloaded from regular host code. I think SYCLDevice attribute actually 
doesn't need a spelling because it will be added only implicitly by compiler.



Comment at: clang/include/clang/Basic/Attr.td:1003
+  let LangOpts = [SYCL];
+  let Documentation = [Undocumented];
+}

aaron.ballman wrote:
> No new, undocumented attributes, please.
As I said, these attributes are not for public consumption. Should I add 
documentation in this case too?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 attribute.
>  I can't just use CUDA-specific attributes for SYCL but I think that these 
> resources for marking device code could be unified and re-used for all 
> supported single-source offload programming models (like SYCL, CUDA, OpenMP 
> etc).
>  Please let me know If you have any suggestions about it.


The problem is that all thee attributes have different semantics. 
OpenMP-specific attributes, for example, cannot be specified explcitly as the 
attributes, because according to the standard they are represented as pragmas. 
Because of thatmost of the OpenMP attributes can be created only implicitly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 for marking device code could be unified and re-used for all 
supported single-source offload programming models (like SYCL, CUDA, OpenMP 
etc).
Please let me know If you have any suggestions about it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 {
+  let Spellings = [GNU<"sycl_device">];
+  let Subjects = SubjectList<[Function, Var]>;

Is there a reason to not also introduce a C++11 and C2x style spelling in the 
`clang` namespace? e.g., `[[clang::sycl_device]]`



Comment at: clang/include/clang/Basic/Attr.td:1003
+  let LangOpts = [SYCL];
+  let Documentation = [Undocumented];
+}

No new, undocumented attributes, please.



Comment at: clang/test/SemaSYCL/kernel-attribute.cpp:4-5
+
+__attribute((sycl_kernel)) void foo() {
+}

Missing some tests:
* test that both attributes can be applied to whatever subjects they appertain 
to
* test that neither attribute can be applied to an incorrect subject
* test that the attributes do not accept arguments
* test that the attribute is ignored when SYCL is not enabled

Are there situations where the attribute does not make sense, such as member 
functions, virtual functions, etc? If so, those are good test cases (and 
diagnostics) to add as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[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 understand if there are other functions/variables
which are supposed to be compiled for the device and mark them with
sycl_device attribute.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60455

Files:
  clang/include/clang/Basic/Attr.td
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/SemaSYCL/kernel-attribute.cpp


Index: clang/test/SemaSYCL/kernel-attribute.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/kernel-attribute.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -verify -pedantic %s
+// expected-no-diagnostics
+
+__attribute((sycl_kernel)) void foo() {
+}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -124,6 +124,8 @@
 // CHECK-NEXT: ReturnTypestate (SubjectMatchRule_function, 
SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ReturnsNonNull (SubjectMatchRule_objc_method, 
SubjectMatchRule_function)
 // CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)
+// CHECK-NEXT: SYCLDevice (SubjectMatchRule_function, 
SubjectMatchRule_variable)
+// CHECK-NEXT: SYCLKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ScopedLockable (SubjectMatchRule_record)
 // CHECK-NEXT: Section (SubjectMatchRule_function, 
SubjectMatchRule_variable_is_global, SubjectMatchRule_objc_method, 
SubjectMatchRule_objc_property)
 // CHECK-NEXT: SetTypestate (SubjectMatchRule_function_is_member)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6755,6 +6755,9 @@
   case ParsedAttr::AT_Flatten:
 handleSimpleAttribute(S, D, AL);
 break;
+  case ParsedAttr::AT_SYCLKernel:
+handleSimpleAttribute(S, D, AL);
+break;
   case ParsedAttr::AT_Format:
 handleFormatAttr(S, D, AL);
 break;
Index: clang/include/clang/Basic/Attr.td
===
--- clang/include/clang/Basic/Attr.td
+++ clang/include/clang/Basic/Attr.td
@@ -291,6 +291,7 @@
 def MicrosoftExt : LangOpt<"MicrosoftExt">;
 def Borland : LangOpt<"Borland">;
 def CUDA : LangOpt<"CUDA">;
+def SYCL : LangOpt<"SYCLIsDevice">;
 def COnly : LangOpt<"CPlusPlus", 1>;
 def CPlusPlus : LangOpt<"CPlusPlus">;
 def OpenCL : LangOpt<"OpenCL">;
@@ -995,6 +996,20 @@
   let Documentation = [Undocumented];
 }
 
+def SYCLDevice : InheritableAttr {
+  let Spellings = [GNU<"sycl_device">];
+  let Subjects = SubjectList<[Function, Var]>;
+  let LangOpts = [SYCL];
+  let Documentation = [Undocumented];
+}
+
+def SYCLKernel : InheritableAttr {
+  let Spellings = [GNU<"sycl_kernel">];
+  let Subjects = SubjectList<[Function]>;
+  let LangOpts = [SYCL];
+  let Documentation = [Undocumented];
+}
+
 def C11NoReturn : InheritableAttr {
   let Spellings = [Keyword<"_Noreturn">];
   let Subjects = SubjectList<[Function], ErrorDiag>;


Index: clang/test/SemaSYCL/kernel-attribute.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/kernel-attribute.cpp
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -fsycl-is-device -fsyntax-only -verify -pedantic %s
+// expected-no-diagnostics
+
+__attribute((sycl_kernel)) void foo() {
+}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -124,6 +124,8 @@
 // CHECK-NEXT: ReturnTypestate (SubjectMatchRule_function, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ReturnsNonNull (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)
+// CHECK-NEXT: SYCLDevice (SubjectMatchRule_function, SubjectMatchRule_variable)
+// CHECK-NEXT: SYCLKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ScopedLockable (SubjectMatchRule_record)
 // CHECK-NEXT: Section (SubjectMatchRule_function, SubjectMatchRule_variable_is_global, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
 // CHECK-NEXT: SetTypestate (SubjectMatchRule_function_is_member)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6755,6 +6755,9 @@
   case ParsedAttr::AT_Flatt