[SYCL] Add support for sycl_special_class attribute

[zahiraam]

Summary

Special classes such as accessor, sampler, and stream need additional implementation when they are passed from host to device.
This patch is adding a new attribute “sycl_special_class” used to mark SYCL classes/struct that need the additional compiler handling.

[Naghasan]

Just a few comments

clang/include/clang/Basic/AttrDocs.td

411–413

I think this is a bit hard to follow if you don't know the SYCL kernel lowering process. Small suggestion to try to make this more self contained.

clang/lib/Sema/SemaDeclAttr.cpp

8058

Don't we need to have a validation for at least the __init function ? The doc says it is mandatory.

[Fznamznon]

clang/lib/Sema/SemaDecl.cpp

9118 ↗

(On Diff #393532)

In our downstream we have not only __init, but __init_esimd as well there. I wonder how this case should be handled?

16675–16683 ↗

(On Diff #393532)

Code style correction.

clang/lib/Sema/SemaDeclAttr.cpp

7636

I wonder if this check is enforced by this line in Attr.td:

let Subjects = SubjectList<[CXXRecord]>;

I would assume that it is.

clang/test/SemaSYCL/special-class-attribute.cpp

22

Did you mean struct keyword here?

32

Is this one needed?

44

Will detection of __init method work correctly if it is defined outside of the class?
Maybe we should also mention that it should be not be defined in a separate library.

[zahiraam]

clang/lib/Sema/SemaDecl.cpp

9118 ↗

(On Diff #393532)

I think the presence of method __init_esimd implies the presence of __init method so we could eventually check first for the presence of __init_esimd and if not present check for the presence of __init?

clang/test/SemaSYCL/special-class-attribute.cpp

44

Both init and finalize must be defined inside the class/struct.

[zahiraam]

@Fznamznon @bader @Naghasan reviews please? Thanks.

[bader]

LGTM, just one suggestion.
It would be great to get @aaron.ballman approve too.

clang/lib/Sema/SemaDecl.cpp

16690 ↗

(On Diff #395176)

I think we might want to check that there is only one member function with __init name to avoid ambiguity with building kernel parameters.

[aaron.ballman]

Thanks! Mostly just minor nits from me.

clang/include/clang/AST/DeclCXX.h

1143 ↗

(On Diff #395176)

clang/include/clang/Basic/AttrDocs.td

411

412

416

417

418

This still isn't quite right -- I don't know how to read "additional information required".

420

421

424

425

428

clang/lib/Sema/SemaDecl.cpp

9126–9127 ↗

(On Diff #395176)

16686–16687 ↗

(On Diff #395176)

I think we can assert there is a definition given that this is called from "ActOnTagFinishDefinition".

[zahiraam]

clang/lib/Sema/SemaDecl.cpp

16686–16687 ↗

(On Diff #395176)

@aaron.ballman Not sure what you mean by "that this is called from". We are inside the ActOnTagFinishDefinition function.

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

16686–16687 ↗

(On Diff #395176)

Sorry for being unclear -- yes, we're inside ActOnTagFinishDefinition() and we just called FinishClass(), so I don't think we need to test if (Def). Instead, I think we can do:

const TagDecl *Def = RD->getDefinition();
assert(Def && "Expected to have a completed definition");
if (!Def->hasInitMethod())

[keryell]

That looks good.

clang/include/clang/Basic/AttrDocs.td

455

[aaron.ballman]

LGTM aside from some small nits.

clang/include/clang/Basic/AttrDocs.td

411

I love the Oxford comma and am not afraid to admit it in public. :-D

418

clang/test/SemaSYCL/special-class-attribute-on-non-sycl.cpp

12

clang/test/SemaSYCL/special-class-attribute.cpp

6

Can you correct this entire file not to have the trailing semicolons?

[Naghasan]

LGTM, thanks for the work

[zahiraam]

Thanks for the reviews.

[MaskRay]

Closed by 8ba9c794feb30cd969b9776c39873def10c51bff.

If the commit message contained Differential Revision:, the differential would be closely automatically when you pushed it to the git repo.
Please ensure the tag is contained next time. arc diff adds this tag automatically.

clang/include/clang/Basic/AttrDocs.td had some formatting issues and caused (-DLLVM_ENABLE_SPHINX=ON) ninja docs-clang-html to fail.
I fixed it. Please test this build target for .rst and AttrDocs.td changes.

[zahiraam]

In D114483#3280905, @MaskRay wrote:

Closed by 8ba9c794feb30cd969b9776c39873def10c51bff.

If the commit message contained Differential Revision:, the differential would be closely automatically when you pushed it to the git repo.
Please ensure the tag is contained next time. arc diff adds this tag automatically.

clang/include/clang/Basic/AttrDocs.td had some formatting issues and caused (-DLLVM_ENABLE_SPHINX=ON) ninja docs-clang-html to fail.
I fixed it. Please test this build target for .rst and AttrDocs.td changes.

@MaskRay OK thanks. I see other clang-format issues. Should those be fixed too?

[aaron.ballman]

In D114483#3281714, @zahiraam wrote:

In D114483#3280905, @MaskRay wrote:

Closed by 8ba9c794feb30cd969b9776c39873def10c51bff.

If the commit message contained Differential Revision:, the differential would be closely automatically when you pushed it to the git repo.
Please ensure the tag is contained next time. arc diff adds this tag automatically.

clang/include/clang/Basic/AttrDocs.td had some formatting issues and caused (-DLLVM_ENABLE_SPHINX=ON) ninja docs-clang-html to fail.
I fixed it. Please test this build target for .rst and AttrDocs.td changes.

@MaskRay OK thanks. I see other clang-format issues. Should those be fixed too?

The issues aren't from clang-format, they're from the RST file contents not building in sphinx due to formatting issues in AttrDocs.td.

https://lab.llvm.org/buildbot/#/builders/92/builds/20791/steps/5/logs/stdio
https://lab.llvm.org/buildbot/#/builders/92/builds/21098/steps/5/logs/stdio

are two such examples that stem from your patch. Basically, you have to build the attribute documentation (converting AttrDocs.td into AttributeReference.rst) and then build the sphinx documents locally to ensure you're not introducing new issues. Or, you can try to speculatively fix based on the errors the bot gives you, but sometimes those errors are super cryptic and take multiple tries to fix (like the "can't lex as C++" one above), which is why trying to reproduce locally is a good first step.

[aaron.ballman]

In D114483#3281718, @aaron.ballman wrote:

In D114483#3281714, @zahiraam wrote:

In D114483#3280905, @MaskRay wrote:

Closed by 8ba9c794feb30cd969b9776c39873def10c51bff.

If the commit message contained Differential Revision:, the differential would be closely automatically when you pushed it to the git repo.
Please ensure the tag is contained next time. arc diff adds this tag automatically.

clang/include/clang/Basic/AttrDocs.td had some formatting issues and caused (-DLLVM_ENABLE_SPHINX=ON) ninja docs-clang-html to fail.
I fixed it. Please test this build target for .rst and AttrDocs.td changes.

@MaskRay OK thanks. I see other clang-format issues. Should those be fixed too?

The issues aren't from clang-format, they're from the RST file contents not building in sphinx due to formatting issues in AttrDocs.td.

https://lab.llvm.org/buildbot/#/builders/92/builds/20791/steps/5/logs/stdio
https://lab.llvm.org/buildbot/#/builders/92/builds/21098/steps/5/logs/stdio

are two such examples that stem from your patch. Basically, you have to build the attribute documentation (converting AttrDocs.td into AttributeReference.rst) and then build the sphinx documents locally to ensure you're not introducing new issues. Or, you can try to speculatively fix based on the errors the bot gives you, but sometimes those errors are super cryptic and take multiple tries to fix (like the "can't lex as C++" one above), which is why trying to reproduce locally is a good first step.

I've pushed a fix for it in a10ff373ddfa82a67c580a87f6258aa6ab8dd595 and the sphinx builder is happy again.

[zahiraam]

In D114483#3281731, @aaron.ballman wrote:

In D114483#3281718, @aaron.ballman wrote:

In D114483#3281714, @zahiraam wrote:

In D114483#3280905, @MaskRay wrote:

Closed by 8ba9c794feb30cd969b9776c39873def10c51bff.

If the commit message contained Differential Revision:, the differential would be closely automatically when you pushed it to the git repo.
Please ensure the tag is contained next time. arc diff adds this tag automatically.

clang/include/clang/Basic/AttrDocs.td had some formatting issues and caused (-DLLVM_ENABLE_SPHINX=ON) ninja docs-clang-html to fail.
I fixed it. Please test this build target for .rst and AttrDocs.td changes.

@MaskRay OK thanks. I see other clang-format issues. Should those be fixed too?

The issues aren't from clang-format, they're from the RST file contents not building in sphinx due to formatting issues in AttrDocs.td.

https://lab.llvm.org/buildbot/#/builders/92/builds/20791/steps/5/logs/stdio
https://lab.llvm.org/buildbot/#/builders/92/builds/21098/steps/5/logs/stdio

are two such examples that stem from your patch. Basically, you have to build the attribute documentation (converting AttrDocs.td into AttributeReference.rst) and then build the sphinx documents locally to ensure you're not introducing new issues. Or, you can try to speculatively fix based on the errors the bot gives you, but sometimes those errors are super cryptic and take multiple tries to fix (like the "can't lex as C++" one above), which is why trying to reproduce locally is a good first step.

I've pushed a fix for it in a10ff373ddfa82a67c580a87f6258aa6ab8dd595 and the sphinx builder is happy again.

Thanks @aaron.ballman !

clang/include/clang/Basic/Attr.td

 
 def SYCLKernel : InheritableAttr {
   let Spellings = [Clang<"sycl_kernel">];
   let Subjects = SubjectList<[FunctionTmpl]>;
   let LangOpts = [SYCL];
   let Documentation = [SYCLKernelDocs];
 }
 
+def SYCLSpecialClass: InheritableAttr {
+  let Spellings = [Clang<"sycl_special_class">];
+  let Subjects = SubjectList<[CXXRecord]>;
+  let LangOpts = [SYCL];
+  let Documentation = [SYCLSpecialClassDocs];
+}
+
 def C11NoReturn : InheritableAttr {
   let Spellings = [Keyword<"_Noreturn">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let SemaHandler = 0;
   let Documentation = [C11NoReturnDocs];
 }
 
 def CXX11NoReturn : InheritableAttr {

clang/include/clang/Basic/AttrDocs.td

- The function must have at least one parameter. The first parameter is
   parameters.
 - The function must return void. The compiler reuses the body of marked functions to
   generate the OpenCL kernel body, and the OpenCL kernel must return ``void``.
 
 The SYCL kernel in the previous code sample meets these expectations.
   }];
 }
 
+def SYCLSpecialClassDocs : Documentation {
+  let Category = DocCatStmt;
+  let Content = [{
+The ``__attribute__((sycl_special_class))`` attribute is used in SYCL
+headers to indicate that a class or a struct needs additional implementation when
+it is passed from host to device.
+Special classes/struct will have a mandatory __init method and an optional __finalize
+method (__finalize method method is used only with stream type). __init method gives the
+user additional information required. For instance, the kernel function arguments
+list is derived from the arguments of the __init method. The arguments of the __init method
+are copied into the kernel function argument list and __init and __finalize methods are called
+at the beginning and the end of the kernel, respectively.
+Please note that this is an attribute that is used for internal implementation and not intended
+to be used by external users.
+
+The sntax of the attribute is as follows:
+
+.. code-block:: c++
+
+   class __attribute__((sycl_special_class)) accessor {};
+   class [[clang::sycl_special_class]] accessor {};
+
+This is a code example that illustrates the use of the attribute:
+
+.. code-block:: c++
+
+   class __attribute__((sycl_special_class)) SpecialType {
+   int F1;
+   int F2;
+   void __init(int f1) {
+     F1 = f1;
+     F2 = f1;
+   }
+   void __finalize() {}
+   public:
+     SpecialType() = default;
+     int getF2() const { return F2; }
+   };
+
+   int main () {
+   SpecialType T;
+   cgh.single_task([=]() {
+     T.getF2();
+   });
+}
+
+This would trigger the following kernel entry point in the AST:
+
+.. code-block:: c++
+
+   void __sycl_kernel(int f1) {
+   SpecialType T;
+   T.__init(f1);
+   ...
+   T.__finalize()
+ }
+  }];
+}
+
 def C11NoReturnDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 A function declared as ``_Noreturn`` shall not return to its caller. The
 compiler will generate a diagnostic for a function declared as ``_Noreturn``
 that appears to be capable of returning to its caller. Despite being a type
 specifier, the ``_Noreturn`` attribute cannot be specified on a function
 pointer type.

clang/lib/Sema/SemaDeclAttr.cpp

if (!RetTy->isVoidType()) {
     return;
   }
 
   handleSimpleAttribute<SYCLKernelAttr>(S, D, AL);
 }
 
 static void handleDestroyAttr(Sema &S, Decl *D, const ParsedAttr &A) {
   if (!cast<VarDecl>(D)->hasGlobalStorage()) {
     S.Diag(D->getLocation(), diag::err_destroy_attr_on_non_static_var)
         << (A.getKind() == ParsedAttr::AT_AlwaysDestroy);
     return;
   }
 
   if (A.getKind() == ParsedAttr::AT_AlwaysDestroy)
     handleSimpleAttribute<AlwaysDestroyAttr>(S, D, A);
   else
     handleSimpleAttribute<NoDestroyAttr>(S, D, A);
case ParsedAttr::AT_OptimizeNone:
     handleOptimizeNoneAttr(S, D, AL);
     break;
   case ParsedAttr::AT_EnumExtensibility:
     handleEnumExtensibilityAttr(S, D, AL);
     break;
   case ParsedAttr::AT_SYCLKernel:
     handleSYCLKernelAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_SYCLSpecialClass:
+    handleSimpleAttribute<SYCLSpecialClassAttr>(S, D, AL);
+    break;
   case ParsedAttr::AT_Format:
     handleFormatAttr(S, D, AL);
     break;
   case ParsedAttr::AT_FormatArg:
     handleFormatArgAttr(S, D, AL);
     break;
   case ParsedAttr::AT_Callback:
     handleCallbackAttr(S, D, AL);

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: Pointer (SubjectMatchRule_record_not_is_union)
 // CHECK-NEXT: ReleaseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ReqdWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: Restrict (SubjectMatchRule_function)
 // CHECK-NEXT: ReturnTypestate (SubjectMatchRule_function, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ReturnsNonNull (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)
+// CHECK-NEXT: SYCLSpecialClass (SubjectMatchRule_record)
 // 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)
 // CHECK-NEXT: SpeculativeLoadHardening (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: StandaloneDebug (SubjectMatchRule_record)
 // CHECK-NEXT: SwiftAsync (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: SwiftAsyncContext (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: SwiftAsyncError (SubjectMatchRule_function, SubjectMatchRule_objc_method)

clang/test/SemaSYCL/special-class-attribute-on-non-sycl.cpp

+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fsycl-is-device -verify %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -x c++ %s
+
+#ifndef __SYCL_DEVICE_ONLY__
+// expected-warning@+5 {{'sycl_special_class' attribute ignored}}
+#else
+// expected-no-diagnostics
+#endif
+
+class __attribute__((sycl_special_class)) special_class {};
+

clang/test/SemaSYCL/special-class-attribute.cpp

+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -fsycl-is-device -verify %s
+
+// No diagnostics
+class [[clang::sycl_special_class]] class1 {};
+class __attribute__((sycl_special_class)) class2 {};
+
+class class3;
+class [[clang::sycl_special_class]] class3 {};
+
+class class4;
+class __attribute__((sycl_special_class)) class4 {};
+
+class [[clang::sycl_special_class]] struct1 {};
+struct __attribute__((sycl_special_class)) struct2 {};
+
+class [[clang::sycl_special_class]] class5 {};
+class special5 {};
+
+class __attribute__((sycl_special_class)) class6;
+class class6 {};
+
+
+// Only classes
+[[clang::sycl_special_class]] int var1 = 0; // expected-warning {{'sycl_special_class' attribute only applies to classes}}
+__attribute__((sycl_special_class)) int var2 = 0; // expected-warning {{'sycl_special_class' attribute only applies to classes}}
+
+[[clang::sycl_special_class]] void foo1(); // expected-warning {{'sycl_special_class' attribute only applies to classes}}
+__attribute__((sycl_special_class)) void foo2(); // expected-warning {{'sycl_special_class' attribute only applies to classes}}
+
+// Attribute takes no arguments
+class [[clang::sycl_special_class(1)]] class7 {}; // expected-error {{'sycl_special_class' attribute takes no arguments}}
+class __attribute__((sycl_special_class(1))) class8 {}; // expected-error {{'sycl_special_class' attribute takes no arguments}}
+