[PATCH] D60455: [SYCL] Implement SYCL device code outlining

Mon Jul 1 08:15:11 PDT 2019

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