aaron.ballman added inline comments.
================ Comment at: clang/include/clang/Basic/Attr.td:1034 + let Spellings = [Clang<"sycl_kernel">]; + let Subjects = SubjectList<[Function]>; + let LangOpts = [SYCL]; ---------------- Shouldn't this be `FunctionTemplate` instead? ================ Comment at: clang/include/clang/Basic/AttrDocs.td:260 +The ``sycl_kernel`` attribute specifies that a function will be used by the +compiler to outline device code and to generate OpenCL kernel. +Here is a code example of the SYCL program, which demonstrates compiler's ---------------- generate an OpenCL kernel ================ Comment at: clang/include/clang/Basic/AttrDocs.td:261 +compiler to outline device code and to generate OpenCL kernel. +Here is a code example of the SYCL program, which demonstrates compiler's +outlining job: ---------------- demonstrates the compiler's ================ Comment at: clang/include/clang/Basic/AttrDocs.td:278 +The lambda that is passed to the ``parallel_for`` is called a SYCL "kernel +function". A SYCL "kernel function" defines entry point to the "device part" +of the code. Compiler will traverse all symbols accessible from a ---------------- defines the entry point ================ Comment at: clang/include/clang/Basic/AttrDocs.td:279 +function". A SYCL "kernel function" defines entry point to the "device part" +of the code. Compiler will traverse all symbols accessible from a +"kernel function" and add them to the "device part" of the code. In this code ---------------- The compiler will ================ Comment at: clang/include/clang/Basic/AttrDocs.td:281 +"kernel function" and add them to the "device part" of the code. In this code +example, compiler will add "foo" function to the "device part" of the code. +More details about compilation of functions for device can be found in the ---------------- the compiler will add the "foo" function ================ Comment at: clang/include/clang/Basic/AttrDocs.td:282 +example, compiler will add "foo" function to the "device part" of the code. +More details about compilation of functions for device can be found in the +SYCL 1.2.1 specification Section 6.4. ---------------- More details about the compilation of functions for the device part can be found ================ Comment at: clang/include/clang/Basic/AttrDocs.td:284 +SYCL 1.2.1 specification Section 6.4. +To show to the compiler entry point to the "device part" of the code SYCL +runtime can use the ``sycl_kernel`` attribute in the following way: ---------------- of the code, the SYCL runtime ================ Comment at: clang/include/clang/Basic/AttrDocs.td:308 + +The compiler will also generate OpenCL kernel using the function marked with the +``sycl_kernel`` attribute. ---------------- generate an OpenCL kernel ================ Comment at: clang/include/clang/Basic/AttrDocs.td:313 + +- Function template with at least two template parameters is expected. The compiler +generates OpenCL kernel and uses first template parameter as unique name to the ---------------- The function must be a template with at least two type template parameters. ================ Comment at: clang/include/clang/Basic/AttrDocs.td:314 +- Function template with at least two template parameters is expected. The compiler +generates OpenCL kernel and uses first template parameter as unique name to the +generated OpenCL kernel. Host application uses this unique name to invoke the ---------------- generates an OpenCL kernel and uses the first template parameter as a unique name ================ Comment at: clang/include/clang/Basic/AttrDocs.td:315 +generates OpenCL kernel and uses first template parameter as unique name to the +generated OpenCL kernel. Host application uses this unique name to invoke the +OpenCL kernel generated for the ``sycl_kernel_function`` specialized by ---------------- The host application uses ================ Comment at: clang/include/clang/Basic/AttrDocs.td:318 +this name and second template parameter ``KernelType`` (which might be a lambda type). +- Function must have at least one parameter. First parameter expected to be a +function object type (named or unnamed i.e. lambda). Compiler uses function ---------------- The function must The first parameter is required to be a ================ Comment at: clang/include/clang/Basic/AttrDocs.td:319 +- Function must have at least one parameter. First parameter expected to be a +function object type (named or unnamed i.e. lambda). Compiler uses function +object type fields to generate OpenCL kernel parameters. ---------------- The compiler uses the function object type ================ Comment at: clang/include/clang/Basic/AttrDocs.td:321 +object type fields to generate OpenCL kernel parameters. +- Function must return void. Compiler re-uses body of marked function to +generate OpenCL kernel body and OpenCL kernel must return void. ---------------- 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`. I'd move the "The sycl_kernel_function" sentence to its own paragraph rather than as part of the final bullet. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9739-9740 +def warn_sycl_kernel_attribute_invalid : Warning< + "'sycl_kernel' attribute only applies to template funtions with special prototype, " + "please refer 'sycl_kernel' attribute documentation">, InGroup<IgnoredAttributes>; + ---------------- I think this diagnostic should be split out into a few diagnostics that explicitly cover the requirements. Something like: `'sycl_kernel' attribute only applies to a %select{templated function|function returning 'void'|etc}0`. It's best to avoid trying to send users to documentation if we can just tell them explicitly what they did wrong with their code. ================ Comment at: clang/include/clang/Sema/Sema.h:11210 + /// Access to SYCL device function decls. + SmallVectorImpl<Decl *> &syclDeviceFuncs() { return SyclDeviceFunctions; } + ---------------- Can you add a `const` overload that returns a `const` container reference? Also, why return the container rather than returning an iterator range from the container? ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6449 + + // The 'sycl_device' attribute applies only to functions + auto *FD = dyn_cast<FunctionDecl>(D); ---------------- Spurious newline above and missing a full stop at the end of the comment. Comments below are also missing full stops. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6450-6454 + auto *FD = dyn_cast<FunctionDecl>(D); + if (!FD) { + S.Diag(AL.getLoc(), diag::warn_sycl_kernel_attribute_invalid); + return; + } ---------------- You can replace all this with a `cast<FunctionDecl>(D)` because the common attribute handler already verifies the subject is correct. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6456 + + FunctionTemplateDecl *FT = FD->getDescribedFunctionTemplate(); + ---------------- I'd appreciate this being declared as a `const` pointer (same for the other nodes obtained through `FT`). ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6459-6462 + if (!FT) { + S.Diag(AL.getLoc(), diag::warn_sycl_kernel_attribute_invalid); + return; + } ---------------- If you switch the subject to `FunctionTemplate`, then I believe this predicate can also go away. ================ Comment at: clang/lib/Sema/SemaSYCL.cpp:65-66 + MarkDeviceFunction Marker(*this); + for (const auto &Elt : syclDeviceFuncs()) { + if (auto *Func = dyn_cast<FunctionDecl>(Elt)) { + if (FunctionDecl *Def = Func->getDefinition()) { ---------------- I think there's some type confusion happening here. I would expect `Elt` to either be ` auto *` or `Func` to be a `const auto &`. I suspect `Elt` should be declared as `auto *`. 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