[PATCH] D114483: [SYCL] Add support for sycl_special_class attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks! Mostly just minor nits from me.



================
Comment at: clang/include/clang/AST/DeclCXX.h:1143
 
+  void SetInitMethod(bool val) { data().HasInitMethod = val; }
+  bool hasInitMethod() const { return data().HasInitMethod; }
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:415
+  let Content = [{
+SYCL defines some special classes (accessor, sampler and stream) that require
+specific handling during the generation of SPIR entry point.
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:416
+SYCL defines some special classes (accessor, sampler and stream) that require
+specific handling during the generation of SPIR entry point.
+The ``__attribute__((sycl_special_class))`` attribute is used in SYCL
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:420
+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``
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:421
+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
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:422
+``__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
----------------
This still isn't quite right -- I don't know how to read "additional information required".


================
Comment at: clang/include/clang/Basic/AttrDocs.td:424
+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
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:425
+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.
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:428
+The ``__init`` and ``__finalize`` methods must be defined inside the
+class/struct.
+Please note that this is an attribute that is used for internal implementation
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:429
+class/struct.
+Please note that this is an attribute that is used for internal implementation
+and not intended to be used by external users.
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:432
+
+The sntax of the attribute is as follows:
+
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:9126-9127
+          NewFD->getName() == "__init" && D.isFunctionDefinition()) {
+        if (auto *def = Parent->getDefinition())
+          def->SetInitMethod(true);
+      }
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:16686-16687
+    if (RD->hasAttr<SYCLSpecialClassAttr>()) {
+      auto *Def = RD->getDefinition();
+      if (Def && !Def->hasInitMethod())
+        Diag(RD->getLocation(),
----------------
I think we can assert there is a definition given that this is called from "ActOnTagFinishDefinition".


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

https://reviews.llvm.org/D114483