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