Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[SYCL] Support or diagnose use of namespace std types as kernel type … #1579

Closed
wants to merge 1 commit into from
Closed

[SYCL] Support or diagnose use of namespace std types as kernel type … #1579

wants to merge 1 commit into from

Conversation

premanandrao
Copy link
Contributor

…names

When std::nullptr_t is used as a kernel type, the generated
integration header uses 'nullptr_t'. This causes lookup
errors. Use 'std::nullptr_t' instead.

std::max_align_t is defined (in one implementation) as a typedef
of an anonymous struct. This causes errors when attempting to
forward declare the type in the integration header. Diagnose
such cases earlier.

Signed-off-by: Premanand M Rao premanand.m.rao@intel.com

@erichkeane
Copy link
Contributor

What does the SYCL standard say about the kernel name? Should we just prohibit any non-class or unnamed types?

@premanandrao
Copy link
Contributor Author

What does the SYCL standard say about the kernel name? Should we just prohibit any non-class or unnamed types?

I haven't read the standard in detail, but I found this:
"In SYCL, all
kernels must have a kernel name, which must be a globally-accessible C++ type name."
(https://www.khronos.org/registry/SYCL/specs/sycl-1.2.1.pdf)

@Fznamznon
Copy link
Contributor

Copy paste from the spec.

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, lambdas1 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.

For me It doesn't explicitly say that we cannot use non-class name.

@premanandrao premanandrao force-pushed the remote_fwd_decl_namespace branch from 3f9a10e to e4af722 Compare April 23, 2020 19:20
@premanandrao
[SYCL] Support or diagnose use of namespace std types as kernel type …
46c54c6 
…names

When std::nullptr_t is used as a kernel type, the generated
integration header uses 'nullptr_t'.  This causes lookup
errors.  Use 'std::nullptr_t' instead.

std::max_align_t is defined (in one implementation) as a typedef
of an anonymous struct.  This causes errors when attempting to
forward declare the type in the integration header.  Diagnose
such cases earlier.

Signed-off-by: Premanand M Rao <premanand.m.rao@intel.com>
@premanandrao premanandrao force-pushed the remote_fwd_decl_namespace branch from e4af722 to 46c54c6 Compare April 23, 2020 19:23
erichkeane
erichkeane reviewed Apr 23, 2020
View reviewed changes
Copy link
Contributor

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all for quoting the standard, I think this makes for a tougher solution here...

clang/lib/Sema/SemaSYCL.cpp
// accessible - contradicts the spec
const bool KernelNameIsMissing = TD->getName().empty();
if (KernelNameIsMissing) {
const TagDecl *TD = isa<ClassTemplateDecl>(D)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this needs to be lower-level than this, since would this work with 'int'? Or the other builtin types? I don't think they are tagdecls, but they are probably named decls.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the fundamental types and other builtin types, we don't even get here. We enter here only for those types for which a forward declaration is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. @kbobrovs is most familiar with the Int header stuff, so I'll leave the review to him.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @erichkeane

@rolandschulz
Copy link
Contributor

For me It doesn't explicitly say that we cannot use non-class name.

Discussion has been moved to Khronos

@keryell
Copy link
Contributor

keryell commented Apr 24, 2020

Otherwise you can probably address this by metaprogramming.
You put a globally accessible wrapper type in your header and when the parallel_for see a problematic typename, the type is wrapped in your wrapper.

kbobrovs
kbobrovs approved these changes May 4, 2020
View reviewed changes
Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

rolandschulz
rolandschulz requested changes May 5, 2020
View reviewed changes
Copy link
Contributor

@rolandschulz rolandschulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be merged. See previous comment.

rolandschulz
rolandschulz reviewed May 6, 2020
View reviewed changes
Copy link
Contributor

@rolandschulz rolandschulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just discussed this in the up-streaming call. The improvements of the diagnostic in this change are useful. Adding extra support for std types is not needed.

clang/lib/Sema/SemaSYCL.cpp
@@ -1826,8 +1823,10 @@ void SYCLIntegrationHeader::emit(raw_ostream &O) {
O << "// This is auto-generated SYCL integration header.\n";
O << "\n";

O << "#include <cstddef>\n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need these changes to the generated header (this and the next change). Because there is no promise that std types work as lambda name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@premanandrao, could you address this comment, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bader , @jtmott-intel is working on it.

@jtmott-intel jtmott-intel mentioned this pull request Jun 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erichkeane erichkeane erichkeane left review comments

@bader bader bader left review comments

@rolandschulz rolandschulz rolandschulz requested changes

@kbobrovs kbobrovs kbobrovs approved these changes

@Fznamznon Fznamznon Awaiting requested review from Fznamznon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@premanandrao @erichkeane @Fznamznon @rolandschulz @keryell @bader @kbobrovs