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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: arphaman.
aaron.ballman added a subscriber: arphaman.
aaron.ballman added inline comments.
Herald added a subscriber: dexonsmith.


================
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<
----------------
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)?


================
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)
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455