[PATCH] D60455: [SYCL] Implement SYCL device code outlining
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 18 07:04:22 PDT 2019
aaron.ballman added a comment.
Most of the comments are about minor nits like grammar and coding conventions, but I did have some questions regarding what kinds of functions the sycl_kernel attribute gets applied to. Also, I'd like to see some additional tests that demonstrate the sycl device attribute is being implicitly created on the proper declarations as expected (can probably do that using -ast-dump and checking to see if the right functions have the attribute attached).
================
Comment at: clang/include/clang/Basic/AttrDocs.td:259
+ let Content = [{
+The ``sycl_kernel`` attribute specifies that a function is SYCL "kernel
+function". SYCL "kernel function" defines an entry point to a kernel.
----------------
is SYCL "kernel function" -> is a SYCL "kernel function"
================
Comment at: clang/include/clang/Basic/AttrDocs.td:260
+The ``sycl_kernel`` attribute specifies that a function is SYCL "kernel
+function". SYCL "kernel function" defines an entry point to a kernel.
+Kernel is a function which will be compiled for the device and defines some
----------------
SYCL -> A SYCL
================
Comment at: clang/include/clang/Basic/AttrDocs.td:261
+function". SYCL "kernel function" defines an entry point to a kernel.
+Kernel is a function which will be compiled for the device and defines some
+entry point to device code i.e. will be called by host in run time.
----------------
Kernel is a -> A kernel is a
================
Comment at: clang/include/clang/Basic/AttrDocs.td:263-264
+entry point to device code i.e. will be called by host in run time.
+Here is a code example of the SYCL program, which demonstrates the need for
+this attribute:
+.. code-block:: c++
----------------
This doesn't really demonstrate the need for the attribute -- the attribute is never shown in the code example. I'd prefer an example that shows when and how a user would write this attribute.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:278
+ }
+The lambda that is passed to the ``parallel_for`` is called SYCL
+"kernel function".
----------------
called SYLC -> called a SYLC
================
Comment at: clang/include/clang/Basic/AttrDocs.td:280
+"kernel function".
+The SYCL Runtime implementation will use ``sycl_kernel`` attribute to mark this
+lambda as SYCL "kernel function". Compiler is supposed to construct a kernel
----------------
use sycl_kernel -> use the sycl_kernel
================
Comment at: clang/include/clang/Basic/AttrDocs.td:281
+The SYCL Runtime implementation will use ``sycl_kernel`` attribute to mark this
+lambda as SYCL "kernel function". Compiler is supposed to construct a kernel
+from "kernel function", add it to the "device part" of code and traverse all
----------------
as SYCL -> as a SYCL
Compiler is supposed to -> The compiler will
================
Comment at: clang/include/clang/Basic/AttrDocs.td:284
+symbols accessible from "kernel function" and add them to the "device part" of
+the code. In this code example compiler is supposed to add "foo" function to the
+"device part" of the code.
----------------
In this code example compiler is supposed to add "foo" function -> In this code example, the compiler will add the "foo" function
================
Comment at: clang/include/clang/Sema/Sema.h:11182
+private:
+ /// Contains Function declarations to be added to the SYCL device code.
+ /// In SYCL when we generate device code we don't know which functions we will
----------------
Function -> function
================
Comment at: clang/include/clang/Sema/Sema.h:11183
+ /// Contains Function declarations to be added to the SYCL device code.
+ /// In SYCL when we generate device code we don't know which functions we will
+ /// emit before we emit sycl kernels so we add device functions to this array
----------------
In SYCL, when we generate device code, we don't know
================
Comment at: clang/include/clang/Sema/Sema.h:11184
+ /// In SYCL when we generate device code we don't know which functions we will
+ /// emit before we emit sycl kernels so we add device functions to this array
+ /// and handle it in separate way.
----------------
we emit sycl kernels, so we add device
================
Comment at: clang/include/clang/Sema/Sema.h:11189
+public:
+ /// This function adds function declaration to the SYCL device code.
+ void AddSyclDeviceFunc(Decl *D) {
----------------
adds the function declaration
================
Comment at: clang/include/clang/Sema/Sema.h:11190
+ /// This function adds function declaration to the SYCL device code.
+ void AddSyclDeviceFunc(Decl *D) {
+ D->addAttr(SYCLDeviceAttr::CreateImplicit(Context));
----------------
Should be named `addSyclDeviceFunc()` per coding standards. Similar for the other new functions.
================
Comment at: clang/include/clang/Sema/Sema.h:11194
+ }
+ /// SyclDeviceFuncs - access to SYCL device function decls.
+ SmallVector<Decl *, 4> &SyclDeviceFuncs() { return SyclDeviceFunctions; }
----------------
Don't repeat the function name in the comments, please. Also, rather than returning a concrete `SmallVector<>`, I think it would be more natural to return a `SmallVectorImpl` so that callers don't have to contend with the explicit size. There should also be a `const` overload for this function.
================
Comment at: clang/include/clang/Sema/Sema.h:11197
+
+ /// Constructs SYCL kernel which is compatible with OpenCL from SYCL "kernel
+ /// function" and adds it to the SYCL device code.
----------------
Constructs a SYCL kernel that is compatible with OpenCL from the SYCL "kernel
================
Comment at: clang/include/clang/Sema/Sema.h:11200-11201
+ void ConstructSYCLKernel(FunctionDecl *KernelCallerFunc);
+ /// This function marks all functions accessible from SYCL kernels with SYCL
+ /// device attribute and adds them to the SYCL device code.
+ void MarkDevice(void);
----------------
Marks all functions accessible from SYCL kernels with the SYCL device attribute
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2410
+ // If this is SYCL device, only emit declarations marked with SYCL device
+ // attribute.
----------------
with the SYCL device attribute
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2412-2415
+ if (LangOpts.SYCLIsDevice) {
+ if (!Global->hasAttr<SYCLDeviceAttr>())
+ return;
+ }
----------------
These `if` statements can be combined.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2533
+ // SYCL kernels can be templated and not called from anywhere in the
+ // module but should be emitted
+ addDeferredDeclToEmit(GD);
----------------
Missing a full stop at the end of the comment.
================
Comment at: clang/lib/Sema/SemaSYCL.cpp:14
+
+#include <array>
+
----------------
This include doesn't seem to be necessary?
================
Comment at: clang/lib/Sema/SemaSYCL.cpp:23
+
+ bool VisitCallExpr(CallExpr *e) {
+ if (FunctionDecl *Callee = e->getDirectCallee()) {
----------------
`e` does not use our usual naming conventions.
================
Comment at: clang/lib/Sema/SemaSYCL.cpp:41
+ bool VisitCXXConstructExpr(CXXConstructExpr *E) {
+
+ CXXConstructorDecl *Ctor = E->getConstructor();
----------------
Spurious whitespace can be removed.
================
Comment at: clang/lib/Sema/SemaSYCL.cpp:44
+
+ if (FunctionDecl *Def = Ctor->getDefinition()) {
+ SemaRef.AddSyclDeviceFunc(Def);
----------------
Elide braces.
================
Comment at: clang/lib/Sema/SemaSYCL.cpp:48
+
+ const auto *ConstructedType = Ctor->getParent();
+ if (ConstructedType->hasUserDeclaredDestructor()) {
----------------
Don't use `auto` as the type is not spelled out in the initialization.
================
Comment at: clang/lib/Sema/SemaSYCL.cpp:52
+
+ if (FunctionDecl *Def = Dtor->getDefinition()) {
+ SemaRef.AddSyclDeviceFunc(Def);
----------------
Elide braces.
================
Comment at: clang/lib/Sema/SemaSYCL.cpp:68
+void Sema::MarkDevice(void) {
+ // Let's mark all called functions with SYCL Device attribute.
+ MarkDeviceFunction Marker(*this);
----------------
with the SYCL device attribute
================
Comment at: clang/lib/Sema/SemaSYCL.cpp:70
+ MarkDeviceFunction Marker(*this);
+ for (const auto &elt : SyclDeviceFuncs()) {
+ if (FunctionDecl *Func = dyn_cast<FunctionDecl>(elt)) {
----------------
`elt` -> `Elt` per naming conventions
================
Comment at: clang/lib/Sema/SemaSYCL.cpp:71
+ for (const auto &elt : SyclDeviceFuncs()) {
+ if (FunctionDecl *Func = dyn_cast<FunctionDecl>(elt)) {
+ if (FunctionDecl *Def = Func->getDefinition()) {
----------------
`auto *` since the type is spelled out in the initialization.
================
Comment at: clang/lib/Sema/SemaSYCL.cpp:73
+ if (FunctionDecl *Def = Func->getDefinition()) {
+ if (!Def->hasAttr<SYCLDeviceAttr>()) {
+ AddSyclDeviceFunc(Def);
----------------
Elide braces
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:5523
+ // have deferred instantination. We need bodies of these functions
+ // so we are checking for SYCL kernel attribute after instantination.
+ if (getLangOpts().SYCLIsDevice &&
----------------
for the SYCL kernel attribute
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:5525
+ if (getLangOpts().SYCLIsDevice &&
+ CurFD->hasAttr<SYCLKernelAttr>()) {
+ ConstructSYCLKernel(CurFD);
----------------
Elide braces
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:5537
+ // have deferred instantination. We need bodies of these functions
+ // so we are checking for SYCL kernel attribute after instantination.
+ if (getLangOpts().SYCLIsDevice &&
----------------
for the SYCL kernel attribute
================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:5539
+ if (getLangOpts().SYCLIsDevice &&
+ Function->hasAttr<SYCLKernelAttr>()) {
+ ConstructSYCLKernel(Function);
----------------
Elide braces
================
Comment at: clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp:4
+
+#if not defined(__SYCL_DEVICE_ONLY__)
+// expected-warning at +6 {{'sycl_kernel' attribute ignored}}
----------------
`#ifndef` ?
================
Comment at: clang/test/SemaSYCL/device-attributes-on-non-sycl.cpp:11
+
+__attribute((sycl_kernel)) void foo();
+[[clang::sycl_kernel]] void foo2();
----------------
I'd prefer to spell this with `__attribute__`, same in the other test
================
Comment at: clang/test/SemaSYCL/device-attributes.cpp:3
+
+[[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}}
----------------
I'd like to see some more tests covering less obvious scenarios. Can I add this attribute to a lambda? What about a member function? How does it work with virtual functions? That sort of thing.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60455/new/
https://reviews.llvm.org/D60455
More information about the cfe-commits
mailing list