-
Notifications
You must be signed in to change notification settings - Fork 770
[SYCL] Support or diagnose use of namespace std types as kernel type names #1963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
46c54c6
5e8004d
26b38f9
c21b9df
1d345ec
d2d55b0
e678857
e6afe60
07e8899
e52236f
0340297
c7c155f
5752ac2
be84c38
fce4460
8e3ea41
bdee3e1
04bd440
736118f
a96b556
8d0087e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
// RUN: %clang_cc1 -fsycl -fsycl-is-device -fsycl-int-header=%t.h -DCHECK_ERROR -verify %s | ||
// RUN: %clang_cc1 -fsycl -fsycl-is-device -triple spir64-unknown-unknown-sycldevice -fsycl-int-header=%t.h %s | ||
// RUN: FileCheck -input-file=%t.h %s | ||
// | ||
// CHECK: #include <CL/sycl/detail/defines.hpp> | ||
// CHECK-NEXT: #include <CL/sycl/detail/kernel_desc.hpp> | ||
// | ||
// CHECK: static constexpr | ||
// CHECK-NEXT: const char* const kernel_names[] = { | ||
// CHECK-NEXT: "_ZTSm", | ||
// CHECK-NEXT: "_ZTSl" | ||
// CHECK-NEXT: }; | ||
// | ||
// CHECK: static constexpr | ||
// CHECK-NEXT: const kernel_param_desc_t kernel_signatures[] = { | ||
// CHECK-NEXT: //--- _ZTSm | ||
// CHECK-EMPTY: | ||
// CHECK-NEXT: //--- _ZTSl | ||
// CHECK-EMPTY: | ||
// CHECK-NEXT: }; | ||
// | ||
// CHECK: static constexpr | ||
// CHECK-NEXT: const unsigned kernel_signature_start[] = { | ||
// CHECK-NEXT: 0, // _ZTSm | ||
// CHECK-NEXT: 1 // _ZTSl | ||
// CHECK-NEXT: }; | ||
|
||
// CHECK: template <> struct KernelInfo<unsigned long> { | ||
// CHECK: template <> struct KernelInfo<long> { | ||
|
||
void usage() { | ||
} | ||
|
||
namespace std { | ||
typedef long unsigned int size_t; | ||
typedef long int ptrdiff_t; | ||
typedef decltype(nullptr) nullptr_t; | ||
class T; | ||
class U; | ||
} // namespace std | ||
|
||
template <typename T> | ||
struct Templated_kernel_name; | ||
|
||
template <typename name, typename Func> | ||
__attribute__((sycl_kernel)) void kernel_single_task(Func kernelFunc) { | ||
kernelFunc(); | ||
} | ||
|
||
int main() { | ||
#ifdef CHECK_ERROR | ||
kernel_single_task<std::nullptr_t>([=]() {}); // expected-error {{kernel name cannot be a type in the "std" namespace}} | ||
kernel_single_task<std::T>([=]() {}); // expected-error {{kernel name cannot be a type in the "std" namespace}} | ||
kernel_single_task<Templated_kernel_name<std::nullptr_t>>([=]() {}); // expected-error {{kernel name cannot be a type in the "std" namespace}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be allowed. The KernelName here is I think this is why the tests are failing as well. Changing this will probably fix the tests without the need for 'SpecializationKernelName'. I think this change might be breaking valid code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem here is that the justification here is the same. Whether you do kernel_single_taskstd::string or kernel_single_task<some_templatestd::string>, we cannot actually put it in the int-header, because we cannot forward declare std::string. nullptr_t and std::size_t/ptrdiff_t are less troublesome, since we know they are well defined typedefs (and can just spell them that way), but other types we cannot. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@jtmott-intel, if a Kernel Name is not a std-type itself, but is templated on a std-type instead, like: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @premanadrao: How would we get away with that? We cannot spell foo<std::string> without spelling std::string. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In current code, I think std::some_type is also forward declared. There is no check on template parameters IIRC. However, if std header is included prior to integration header, I see no reason why we need to forward declare std::sometype There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think we have to prohibit everything in namespace 'std', with some allowance for the types where desugaring/canonicalizing results in a non-std type (basically, anything that was typedef'ed to a builtin type). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @erichkeane I think that's what we have now in this PR. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No just being clear. I wouldn't mind allowing std::nullptr_t as well, since we can just canonicalize it as decltype(nullptr_t) though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know what to do here. Things need to be forward declarable for us to do the right thing in the int-header. std types aren't. So we don't really have a choice. But we have codes using std-types in the name. Therefore if we go ahead with this solution we'll break codes. And the workaround shown above (with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just talked with @mkinsner . The intention is that that names and their template arguments are forward declarable. This excludes std types. We think it is OK for us to not support names with std names as template arguments. For those use cases users should use unnamed lambdas. |
||
kernel_single_task<Templated_kernel_name<std::U>>([=]() {}); // expected-error {{kernel name cannot be a type in the "std" namespace}} | ||
#endif | ||
|
||
// Although in the std namespace, these resolve to builtins such as `int` that are allowed in kernel names | ||
kernel_single_task<std::size_t>([=]() {}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that any exceptions we have to allow std types like size_t and ptrdiff_t, we need to just canonicalize them. So this should be kernel_single_task. Otherwise we're still violating the same rule as with std::string. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The complication with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you misunderstood my comment (and apparently github ate the 'so this should be kernel_single_task<unsigned long>> line) ... I think we need to prohibit any of the classes in the std namespace, but size_t/ptrdiff_t/etc are both typedefs, so just canonicalizing them (aka, desugaring the typedef) should be sufficient. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. If I understand what you mean, then size_t/ptrdiff_t are already desurgared. A |
||
kernel_single_task<std::ptrdiff_t>([=]() {}); | ||
|
||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole thing is a non-issue if unnamed lambda compiler option is enabled even if the user provides a kernel name. Because then we don't template on the lambda name type but on the string representation of it. Therefore we don't need to emit an error in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we could always do that: Template
KernelInfo
on the string representation of the name rather than the name type. Then we would never need to forward declare the name type.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started experimenting with templating KernelInfo on the string representation of the name, and probably went further down that rabbit hole than I should have before I realized I was reimplementing what the existing unnamed lambda logic does already. And so in this latest commit -- meant to prompt discussion -- I flipped a 0 to a 1 (don't laugh :-P) and turned on the unnamed lambda CLI option by default. This seems to deliver exactly what we've been wanting. KernelInfo is templated on the string representation of the name, nullptr_t and other std names work, and the existing code will already skip forward decls when unnamed lambda is on. I assume, though, that the idea of turning on unnamed lambdas by default was already discussed in some meeting somewhere. Is there some reason it can't be this simple?
@rolandschulz @erichkeane @premanandrao @elizabethandrews
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, we shouldn't emit this diagnostic if UnnamedLambdaSupport