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

Wed Nov 20 12:33:25 PST 2019

bader marked 3 inline comments as done.
bader added inline comments.

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10079-10080
+def warn_sycl_kernel_invalid_template_param_type : Warning<
+  "template parameter of template functions with 'sycl_kernel' attribute must"
+  " be typename">, InGroup<IgnoredAttributes>;
+def warn_sycl_kernel_num_of_function_params : Warning<
----------------
aaron.ballman wrote:
> bader wrote:
> > aaron.ballman wrote:
> > > This diagnostic reads a bit like you cannot do this: `template <class N>` when I think the actual restriction is that you cannot do this: `template <int N>`. Is that correct? If so, I think this could be worded as `template parameter of a function template with the 'sycl_kernel' attribute must be a template type parameter`.
> > > 
> > > Just double-checking, but you also intend to prohibit template template parameters? e.g., you can't do `template <template <typename> typename C>`
> > > This diagnostic reads a bit like you cannot do this: template <class N> when I think the actual restriction is that you cannot do this: template <int N>. Is that correct?
> > 
> > Yes. That is correct.
> > 
> > >  If so, I think this could be worded as template parameter of a function template with the 'sycl_kernel' attribute must be a template type parameter.
> > 
> > Thanks! Applied your wording.
> > 
> > > Just double-checking, but you also intend to prohibit template template parameters? e.g., you can't do template <template <typename> typename C>
> > 
> > Currently we allow following use case: https://github.com/intel/llvm/blob/sycl/clang/test/SemaSYCL/mangle-kernel.cpp. I assume it qualifies as "template type" and not as "template template" parameter. Right?
> > 
> > Quoting SYCL specification $6.2 Naming of kernels (https://www.khronos.org/registry/SYCL/specs/sycl-1.2.1.pdf#page=250).
> > 
> > > SYCL kernels are extracted from C++ source files and stored in an implementation- defined format. In the case of
> > > the shared-source compilation model, the kernels have to be uniquely identified by both host and device compiler.
> > > This is required in order for the host runtime to be able to load the kernel by using the OpenCL host runtime
> > > interface.
> > > From this requirement the following rules apply for naming the kernels:
> > > • The kernel name is a C++ typename.
> > > • The kernel needs to have a globally-visible name. In the case of a named function object type, the name can
> > > be the typename of the function object, as long as it is globally-visible. In the case where it isn’t, a globally visible name has to be provided, as template parameter to the kernel invoking interface, as described in 4.8.5.
> > > In C++11, lambdas do not have a globally-visible name, so a globally-visible typename has to be provided
> > > in the kernel invoking interface, as described in 4.8.5.
> > > • The kernel name has to be a unique identifier in the program.
> > > 
> > 
> > We also have an extension, which lifts these restrictions/requirements when clang is used as host and device compiler. @erichkeane implemented built-in function (https://github.com/intel/llvm/pull/250) providing "unique identifier", which we use as a kernel name for lambda objects. But this is going to be a separate patch.
> > Currently we allow following use case: https://github.com/intel/llvm/blob/sycl/clang/test/SemaSYCL/mangle-kernel.cpp. I assume it qualifies as "template type" and not as "template template" parameter. Right?
> 
> Yeah, those are template types. A template template parameter would be: https://godbolt.org/z/9kwbW9
> In that example, `C` is a template template parameter and `Ty` is a template type parameter. The part I'm a bit unclear on is why a template template parameter should be disallowed (I believe it names a type, as opposed to a non-type template parameter which names a value)?
I think Mariya implemented this restriction to avoid usages not required for SYCL kernel support implementation in run-time library. As it was mentioned before, this attribute is intended to be used by SYCL run-time library only and current implantation do not require `template template parameter` support.
I think that this might be useful for alternative implementations, so I updated the patch to restrict non-type template parameters only.

================
Comment at: clang/test/Misc/pragma-attribute-supported-attributes-list.test:134
 // CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)
+// CHECK-NEXT: SYCLKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ScopedLockable (SubjectMatchRule_record)
----------------
aaron.ballman wrote:
> bader wrote:
> > It looks like this change is not needed anymore. This check fails on my machine with the latest version of the patch.
> > 
> > @aaron.ballman, I'm not sure if this is a problem of the implementation or test issue.
> > Do I understand correctly that this test validates the list of the attributes which can be applied using `#pragma clang`?
> > If so, removing this check seems to be okay. We need only `[[clang::sycl_kernel]]` or `__attribute__((sycl_kernel))` to work.
> Your understanding is correct, and I think it's a bug if you don't need to add an entry here for `SYCLKernel`. @arphaman, WDYT?
I turned out that the workaround I added to allow only function templates affected this test (described in this comment https://reviews.llvm.org/D60455#1742083).

I.e.
def FunctionTmpl
    : SubsetSubject<Function, [{S->getTemplatedKind() ==
                                 FunctionDecl::TK_FunctionTemplate}],
                    "function templates">;

I also noted that there is no check for `artificial` attribute which uses the same approach to limit the subject to "inline functions".

https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/Attr.td#L652
https://github.com/llvm/llvm-project/blob/master/clang/include/clang/Basic/Attr.td#L122

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455