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] Allow use of function declarators with empty parentheses while trying to compile code for SYCL devices #1994

Merged
merged 8 commits into from
Jul 7, 2020
Merged

[SYCL] Allow use of function declarators with empty parentheses while trying to compile code for SYCL devices #1994

merged 8 commits into from
Jul 7, 2020

Conversation

srividya-sundaram
Copy link
Contributor

@srividya-sundaram srividya-sundaram commented Jun 26, 2020 •
edited
Loading

Treat foo() function as foo(void) instead of a no-prototype function when code is compiled for SYCL devices.

@srividya-sundaram
[SYCL]Cast Integer to Enumeration Type using static_cast for Kernels …
a82639f 
…defined as templated function object where the template parameter is of enum type (scoped and unscoped).

Eg: template <> struct KernelInfo<::dummy_functor_1<(no_namespace_int)0>> to
template <> struct KernelInfo<::dummy_functor_1<static_cast<no_namespace_int>(0)>>
@srividya-sundaram
Merge branch 'sycl' of https://github.com/intel/llvm into sycl
50d5ad9
@srividya-sundaram
Merge branch 'sycl' of https://github.com/intel/llvm into sycl
5ed91c7
@srividya-sundaram
Merge remote-tracking branch 'upstream/sycl' into sycl
db7c20e
@srividya-sundaram
[SYCL] Emit deferred diagnostic if a no-prototype function is invoked…
4921f01 
… from SYCL kernel

In C language, declaring a function without any information about its parameters is allowed.
However, SYCL kernel does not support calling a no-prototype function.
This commit checks if a no-prototype function is present in device code
and if found emits a deferred diagnostic error.
@Fznamznon
Copy link
Contributor

Fznamznon commented Jun 29, 2020 •
edited
Loading

However, SYCL kernel does not support calling a no-prototype function.

There is no such restriction in SYCL spec. It might be connected with spir targets that we use for device compilation. The following code:

file_name.c:
int square();

compiles successfully with the command clang file_name.c. However it cannot be compiled once I add spir64 triple, i.e. clang -target spir64 -emit-llvm -c test.c gives the same error that you mentioned above. In addition, once I pretend that I compile OpenCL C code by adding -x cl flag, everything compiles fine.
I think the approach that you are using is incorrect, we need to do it same as OpenCL does.
Probably this patch will help https://reviews.llvm.org/D33681 .

premanandrao
premanandrao reviewed Jun 29, 2020
View reviewed changes
@srividya-sundaram
Copy link
Contributor Author

srividya-sundaram commented Jun 29, 2020 •
edited
Loading

However, SYCL kernel does not support calling a no-prototype function.

There is no such restriction in SYCL spec. It might be connected with spir targets that we use for device compilation. The following code:

file_name.c:
int square();

compiles successfully with the command clang file_name.c. However it cannot be compiled once I add spir64 triple, i.e. clang -target spir64 -emit-llvm -c test.c gives the same error that you mentioned above. In addition, once I pretend that I compile OpenCL C code by adding -x cl flag, everything compiles fine.
I think the approach that you are using is incorrect, we need to do it same as OpenCL does.
Probably this patch will help https://reviews.llvm.org/D33681 .

Thanks for the clarifications.
I tried the same approach implemented for OpenCL, where they treat 'foo()' as 'foo(void)' rather than a function without a prototype. 'HasProto' is set to true for C17 language option and we no longer get the 'function with no prototype' error even when spir64 triple is added during compilation. I chose C17 since it is the latest. If this approach is okay, do we need to add C99/C11 to the list as well?
Edit: I notice that this change cause some tests to fail.

@srividya-sundaram
[SYCL] Allow function declaration with empty argument list.
56b8081 
Treat 'f()' as 'f(void)' rather than a function w/o a prototype.
@srividya-sundaram srividya-sundaram changed the title [SYCL] Emit deferred diagnostic if a no-prototype function is invoked from SYCL kernel [SYCL] Allow use of function declarators with empty parentheses in C Jun 30, 2020
@srividya-sundaram
[SYCL] Allow function declaration with empty argument list
d29eb73 
If a no-prototype function f() is compiled for a SYCL device,then
treat 'f()' as 'f(void)' rather than a function without a prototype.
@srividya-sundaram srividya-sundaram removed the request for review from asavonic June 30, 2020 19:11
@premanandrao
Copy link
Contributor

@Fznamznon, does this look okay to you?

@elizabethandrews
Copy link
Contributor

LGTM

Fznamznon
Fznamznon reviewed Jul 2, 2020
View reviewed changes
Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

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

Your change doesn't do anything for C anymore, please provide correct description and PR name.

@srividya-sundaram srividya-sundaram changed the title [SYCL] Allow use of function declarators with empty parentheses in C [SYCL] Allow use of function declarators with empty parentheses while trying to compile code for SYCL devices Jul 2, 2020
@bader bader merged commit a4f2182 into intel:sycl Jul 7, 2020
@srividya-sundaram srividya-sundaram deleted the no-prototype-function branch July 10, 2020 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@premanandrao premanandrao premanandrao left review comments

@elizabethandrews elizabethandrews elizabethandrews left review comments

@Fznamznon Fznamznon Fznamznon approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@srividya-sundaram @Fznamznon @premanandrao @elizabethandrews @bader