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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
