-
Notifications
You must be signed in to change notification settings - Fork 736
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
[SYCL] Remove need to mark free functions with SYCL_EXTERNAL attribute #14170
Changes from 55 commits
efb93a5
2cd0d1a
9003bf0
c5c2540
318c528
c091fca
d78a903
a0ed967
26b7b09
fae8dd3
44ef070
51f7072
89ef3dc
9c0b8f0
42478c8
a926383
be9c177
e9f50cc
2dc28f7
1e1aa2c
0a5569b
07ebf0d
be6c168
ef8b031
40fb924
bbdf983
38ad179
85fe22a
46d3e5d
66665af
9f4eded
b8e1665
670cc61
f753c37
36ba16a
7e6aacf
c57f9cd
7b0dcf9
a0f4ddc
06f91cf
b83a080
cbb3f23
db857f2
0475c35
e39f44a
2a90137
74b5d82
6d7a31a
e5edf4c
e0f5b82
5603aef
d803209
3b71b2c
af52693
9d1a87e
2d2ef56
7189420
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,59 @@ | ||
//==---- free_function_attribute_check.cpp ---------------------------------==// | ||
// | ||
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
// See https://llvm.org/LICENSE.txt for license information. | ||
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
// RUN: %clang_cc1 -fsycl-is-device -triple -spir64-unknown-unknown -verify %s | ||
|
||
// expected-no-diagnostics | ||
|
||
// This test checks that non-constexpr values appearing in | ||
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. Can you please update this comment? You can say something along the lines of "This test ensures that the compiler does not crash when functions or methods with add_ir_attributes_function attribute in dependent contexts are checked for the presence of the free function property" 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, I've updated the comment. |
||
// add_ir_attributes_function can be handled when checking whether a function is | ||
// a free function. When all values in the attribute can be handled, then we can | ||
// safely test any candidate function for being a free function. | ||
|
||
template <typename T> constexpr int value() { return 5; } | ||
|
||
// In this struct the function the add_ir_attributes_function values for "S()" | ||
// are as follows. Note that the "value" is represented as a CallExpr. | ||
// `-SYCLAddIRAttributesFunctionAttr 0x562ec6c13390 < col:5, col : 67 > | ||
// | -ConstantExpr 0x562ec6c13440 < col:49 > 'const char[5]' lvalue | ||
// | |-value: LValue <todo> | ||
// | `-StringLiteral 0x562ec6c13160 < col:49 > 'const char[5]' lvalue "name" | ||
// `-CallExpr 0x562ec6c13220 < col:57, col : 66 > '<dependent type>' | ||
// `-UnresolvedLookupExpr 0x562ec6c131a8 < col:57, col : 64 > '<dependent type>' lvalue(ADL) = 'value' 0x562ec6bea700 | ||
// `-TemplateArgument type 'T':'type-parameter-0-0' | ||
// `-TemplateTypeParmType 0x562ec6bea8b0 'T' dependent depth 0 index 0 | ||
// `-TemplateTypeParm 0x562ec6bea860 'T' | ||
|
||
template <typename T> struct S { | ||
#if defined(__SYCL_DEVICE_ONLY__) | ||
[[__sycl_detail__::add_ir_attributes_function("name", value<T>())]] | ||
#endif | ||
S() { | ||
} | ||
}; | ||
|
||
// For the free function "f" the add_ir_attributes_function values are: | ||
// | -SYCLAddIRAttributesFunctionAttr 0x56361c3c3ea8 < line:37 : 32, line : 39 : 15 > | ||
// | |-ConstantExpr 0x56361c3c3f00 < line:38 : 5 > 'const char[5]' lvalue | ||
// | | |-value: LValue <todo> | ||
// | | `-StringLiteral 0x56361c398cf0 < col:5 > 'const char[5]' lvalue "name" | ||
// | `-ConstantExpr 0x56361c3c3f60 < line:39 : 5, col : 14 > 'int' | ||
// | |-value: Int 5 | ||
// | `-CallExpr 0x56361c3c3e88 < col:5, col : 14 > 'int' | ||
// | `-ImplicitCastExpr 0x56361c3c3e70 < col:5, col : 12 > 'int (*)()' < FunctionToPointerDecay > | ||
// | `-DeclRefExpr 0x56361c3c3dc0 < col:5, col : 12 > 'int ()' lvalue Function 0x56361c3c3cc8 'value' 'int ()' (FunctionTemplate 0x56361c398a90 'value') | ||
|
||
template <typename T> | ||
__attribute__((sycl_device)) [[__sycl_detail__::add_ir_attributes_function( | ||
"name", | ||
value<T>())]] [[__sycl_detail__:: | ||
add_ir_attributes_function("sycl-single-task-kernel", | ||
0)]] void | ||
f(T i) {} | ||
|
||
template void f(int i); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
//==---- free_function_errors.cpp --------------------------------------==// | ||
// | ||
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
// See https://llvm.org/LICENSE.txt for license information. | ||
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
// RUN: %clangxx -fsyntax-only -fsycl-device-only -Xclang -verify -Xclang -verify-ignore-unexpected=note %s | ||
|
||
#include <array> | ||
#include <sycl/sycl.hpp> | ||
|
||
using namespace sycl; | ||
|
||
struct S { | ||
int i; | ||
float f; | ||
}; | ||
|
||
union U { | ||
int i; | ||
float f; | ||
}; | ||
|
||
using accType = accessor<int, 1, access::mode::read_write>; | ||
|
||
// expected-error@+3 {{'struct S' cannot be used as the type of a kernel parameter}} | ||
SYCL_EXT_ONEAPI_FUNCTION_PROPERTY( | ||
(ext::oneapi::experimental::single_task_kernel)) | ||
void ff(struct S s) {} | ||
|
||
// expected-error@+3 {{'union U' cannot be used as the type of a kernel parameter}} | ||
SYCL_EXT_ONEAPI_FUNCTION_PROPERTY( | ||
(ext::oneapi::experimental::single_task_kernel)) | ||
void ff(union U u) {} | ||
|
||
// expected-error@+3 {{'accType' (aka 'accessor<int, 1, access::mode::read_write>') cannot be used as the type of a kernel parameter}} | ||
SYCL_EXT_ONEAPI_FUNCTION_PROPERTY( | ||
(ext::oneapi::experimental::single_task_kernel)) | ||
void ff(accType acc) {} | ||
|
||
// expected-error@+3 {{'std::array<int, 10>' cannot be used as the type of a kernel parameter}} | ||
SYCL_EXT_ONEAPI_FUNCTION_PROPERTY( | ||
(ext::oneapi::experimental::single_task_kernel)) | ||
void ff(std::array<int, 10> a) {} | ||
|
||
// expected-error@+3 {{'int &' cannot be used as the type of a kernel parameter}} | ||
SYCL_EXT_ONEAPI_FUNCTION_PROPERTY( | ||
(ext::oneapi::experimental::single_task_kernel)) | ||
void ff(int &ip) {} |
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.
Based on your tests below and explanation in PR comments, it sounds like this issue does not have anything to do with
constexpr
values. The issue here is we have non-instantiated functions/methods being checked when they shouldn't be. Can you try replacing this check withLangOpts.SYCLIsDevice && Body && FD->isDependentContext()
without theTemplatedKind
checks. You also do not need theif (FD)
since L16305 above checks this already.I think you should also replace the
hasDependentExpr
checks you added in SemaSYCL to asserts instead because we should not be having dependent arguments at that point.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.
Slight correction, that should be
!FD->isDependentContext()
-- basically, if the function isn't dependent, it's safe to check the arguments; if the function IS dependent somehow, we need to wait to do those checks until the function has been instantiated (regardless of whether it's a member function or free function).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
hasDependentExpr
check I added was a check before callinggetAttributeNameValuePairs
because that would assert if all expressions in the attribute were not constexprs. So there is already an assert there. Perhaps an assert before callinggetAttributeNameValuePairs
is not needed?Making the other change in SemaDecl.cpp was fine. The tests all pass.
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. You can remove all that the code you added then if one already exists in
getAttributeNameValuePairs
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.
Done.