-
Notifications
You must be signed in to change notification settings - Fork 768
[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 11 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 |
---|---|---|
|
@@ -2244,34 +2244,38 @@ void SYCLIntegrationHeader::emitFwdDecl(raw_ostream &O, const Decl *D, | |
auto *NS = dyn_cast_or_null<NamespaceDecl>(DC); | ||
|
||
if (!NS) { | ||
if (!DC->isTranslationUnit()) { | ||
const TagDecl *TD = isa<ClassTemplateDecl>(D) | ||
? cast<ClassTemplateDecl>(D)->getTemplatedDecl() | ||
: dyn_cast<TagDecl>(D); | ||
|
||
if (TD && !UnnamedLambdaSupport) { | ||
// defined class constituting the kernel name is not globally | ||
// accessible - contradicts the spec | ||
const bool KernelNameIsMissing = TD->getName().empty(); | ||
if (KernelNameIsMissing) { | ||
const TagDecl *TD = isa<ClassTemplateDecl>(D) | ||
? cast<ClassTemplateDecl>(D)->getTemplatedDecl() | ||
: dyn_cast<TagDecl>(D); | ||
if (!TD) | ||
break; | ||
|
||
const bool KernelNameIsMissing = TD->getName().empty(); | ||
if (KernelNameIsMissing) | ||
Diag.Report(KernelLocation, diag::err_sycl_kernel_incorrectly_named) | ||
<< /* kernel name is missing */ 0; | ||
else if (!DC->isTranslationUnit()) { | ||
// defined class constituting the kernel name is not globally | ||
// accessible - contradicts the spec | ||
if (!UnnamedLambdaSupport) { | ||
if (TD->isCompleteDefinition()) | ||
Diag.Report(KernelLocation, diag::err_sycl_kernel_incorrectly_named) | ||
<< /* kernel name is missing */ 0; | ||
// Don't emit note if kernel name was completely omitted | ||
} else { | ||
if (TD->isCompleteDefinition()) | ||
Diag.Report(KernelLocation, | ||
diag::err_sycl_kernel_incorrectly_named) | ||
<< /* kernel name is not globally-visible */ 1; | ||
else | ||
Diag.Report(KernelLocation, diag::warn_sycl_implicit_decl); | ||
Diag.Report(D->getSourceRange().getBegin(), | ||
diag::note_previous_decl) | ||
<< TD->getName(); | ||
} | ||
<< /* kernel name is not globally-visible */ 1; | ||
else | ||
Diag.Report(KernelLocation, diag::warn_sycl_implicit_decl); | ||
Diag.Report(D->getSourceRange().getBegin(), diag::note_previous_decl) | ||
<< TD->getName(); | ||
} | ||
} | ||
break; | ||
} | ||
|
||
if (NS->isStdNamespace()) { | ||
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. 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 commentThe reason will be displayed to describe this comment to others. Learn more. It seems like we could always do that: Template 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 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? 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. Right, we shouldn't emit this diagnostic if UnnamedLambdaSupport |
||
Diag.Report(KernelLocation, diag::err_sycl_kernel_incorrectly_named) | ||
<< /* name cannot be a type in the std namespace */ 3; | ||
return; | ||
} | ||
|
||
++NamespaceCnt; | ||
const StringRef NSInlinePrefix = NS->isInline() ? "inline " : ""; | ||
NSStr.insert( | ||
|
@@ -2354,8 +2358,13 @@ void SYCLIntegrationHeader::emitForwardClassDecls( | |
; | ||
const CXXRecordDecl *RD = T->getAsCXXRecordDecl(); | ||
|
||
if (!RD) | ||
if (!RD) { | ||
if (T->isNullPtrType()) | ||
Diag.Report(KernelLocation, diag::err_sycl_kernel_incorrectly_named) | ||
<< /* name cannot be a type in the std namespace */ 3; | ||
|
||
return; | ||
} | ||
|
||
// see if this is a template specialization ... | ||
if (const auto *TSD = dyn_cast<ClassTemplateSpecializationDecl>(RD)) { | ||
|
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.
I spend some time looking into these changes, and I don't get why we didn't start emitting "kernel name is missing" on each unnamed lambda, because before your changes this diagnostic was under
if (!UnnamedLambdaSupport)
and it looked correct.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.
The original "if" was
if (TD && !UnnamedLambdaSupport)
, and when this evaluates false, then the else block would still useTD
anyway -- which seems weird. premanandrao's commit refactored the TD check out toif (!TD) break;
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'm still a bit confused about this change. Can you give an example where
if (TD)
evaluates true and KernelNameIsMissing i.e. TD does not have a name?Also, this change is not technically required for 'std' namespace check right? Or am I missing something?
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 think this happens when it is invalid.
It is not needed strictly for the specific 'std' namespace check, but it occurs in those contexts too.
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.
Normally the
if (TD)
check would ensure that a kernel name type is a TagDecl -- that is, one of a struct/union/class/enum -- which would catch kernel name types such asint
. I say "normally", though, because earlier in the call stack, we also ensure that a kernel name type is a CXXRecordDecl -- that is, one of a struct/union/class. Which means, sadly, that theif (TD)
part may actually be dead code. But, it was dead code even before, and independent of, this PR. I had considered doing a clean-up of the existing code, but... I feel like this PR has already exceed its side-quest quota. :-)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.
Yes this is part of what's being checked in, but the original had dead code too, and this part just moves the dead code a couple lines over.
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 see. In that case, I don't mind this being addressed in a separate PR.
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.
OK then sounds like we're opting to not revert the April refactoring? And this PR would stay as is?
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.
Seems like folks still aren't comfortable with those changes, and since they come from an older time in this PR and don't seem to be needed anymore, I pushed a commit to revert that bit of refactoring. Buildbot is running. I'll ping folks when ready for re-review approvals.
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.
The changes @elizabethandrews was concerned about are reverted. All checks passed. Need review approvals again. @premanandrao @rbegam