Skip to content

[SYCL] Turn indirectly-callable property into sycl_device attribute #13819

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

Merged
merged 3 commits into from
May 22, 2024
Merged

[SYCL] Turn indirectly-callable property into sycl_device attribute #13819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
994a9dc
[SYCL] Turn indirectly-callable property into sycl_device attribute
AlexeySachkov May 17, 2024
1026acf
Merge remote-tracking branch 'origin/sycl' into private/asachkov/impl…
AlexeySachkov May 22, 2024
21d443d
Use null terminator literal instead of integer
AlexeySachkov May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions clang/include/clang/Basic/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1768,6 +1768,30 @@ class SYCLAddIRAttrMemberCodeHolder<code Code> {

// Common class for SYCL add_ir_attributes_* attributes.
def SYCLAddIRAttrCommonMembers : SYCLAddIRAttrMemberCodeHolder<[{
static std::optional<std::string>
getValidAttributeNameAsString(const APValue &Value,
const ASTContext &Context,
QualType ValueQType) {
assert(!Value.isLValue());
if (ValueQType->isCharType()) {
char C = static_cast<char>(Value.getInt().getExtValue());
return std::string(&C, 1);
}
if (ValueQType->isArrayType() &&
(ValueQType->getArrayElementTypeNoTypeQual()->isCharType())) {
SmallString<10> StrBuffer;
for (unsigned I = 0; I < Value.getArrayInitializedElts(); ++I) {
const APValue &ArrayElem = Value.getArrayInitializedElt(I);
char C = static_cast<char>(ArrayElem.getInt().getExtValue());
if (C == '\0')
break;
StrBuffer += C;
}
return std::string(StrBuffer);
}
return std::nullopt;
}

static std::optional<std::string>
getValidAttributeNameAsString(const Expr *NameE, const ASTContext &Context) {
if (const auto *NameLiteral = dyn_cast<StringLiteral>(NameE))
Expand All @@ -1782,7 +1806,8 @@ def SYCLAddIRAttrCommonMembers : SYCLAddIRAttrMemberCodeHolder<[{
NameLValue = NameCE->getAPValueResult();

if (!NameLValue.isLValue())
return std::nullopt;
return getValidAttributeNameAsString(NameLValue, Context,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewers: this particular change was done to avoid assertion failure due to invalid attribute name (nullopt being returned) on the following test case:

llvm/clang/test/SemaSYCL/attr-add-ir-attributes.cpp

Line 78 in c02a98c

[[__sycl_detail__::add_ir_attributes_function(CEAttrName1, nullptr)]] void FunctionCEName1() {}

Copy link
Contributor

@elizabethandrews elizabethandrews May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm a little confused. Why did your change cause this and the test mentioned below to break? I understand getFilteredAttributeNameValuePairs is now called earlier but I don't understand why that causes these specific issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little confused. Why did your change cause this and the test mentioned below to break?

I honestly don't understand that myself. I can do a bit more of a debugging here to see why we see this particular AST and not another. My current guess is that it can be related to evaluation of constexpr variables, i.e. by the time we parse this attribute, we haven't done some processing of CEAttrName1 (due to lazy nature of that processing), but it is always done before we reach CodeGen and therefore we haven't seen this problem before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I seem to have figured it out. The issue is reproducible even without my changes, it is just hidden by the test without my changes. If you try to compile the following file with plain intel/llvm:

$ cat p.cpp
constexpr const char CEAttrName1[] = "CEAttr1";

__attribute__((sycl_device)) [[__sycl_detail__::add_ir_attributes_function(CEAttrName1, nullptr)]] void FunctionCEName1() {}

$ clang++ -cc1 -fsycl-is-device -fsyntax-only p.cpp
clang++: github.com/intel/llvm/build/tools/clang/include/clang/AST/Attrs.inc:11988: llvm::SmallVector<std::pair<std::__cxx11::basic_strin
g<char>, std::__cxx11::basic_string<char> >, 4> clang::SYCLAddIRAttributesFunctionAttr::getFilteredAttributeNameValuePairs(const std::optional<llvm::SmallSet<
llvm::StringRef, 4> >&, const clang::ASTContext&) const: Assertion `NameStr && "Attribute name is not a valid string."' failed.
PLEASE submit a bug report to https://github.com/intel/llvm/issues and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: ./bin/clang++ -cc1 -fsycl-is-device -fsyntax-only p.cpp
1.      <eof> parser at end of file
2.      p.cpp:3:123: parsing function body 'FunctionCEName1'

Because the test also contains lines that cause compilation errors, we exit early from parsing/handling some of the things and therefore we don't see this assert. With the attribute arguments analysis performed earlier, we always go through that path and any "early exits" due to errors in the code do not help us avoid this issue.

NameCE->getType());

if (const auto *NameValExpr =
NameLValue.getLValueBase().dyn_cast<const Expr *>())
Expand Down Expand Up @@ -1816,10 +1841,10 @@ def SYCLAddIRAttrCommonMembers : SYCLAddIRAttrMemberCodeHolder<[{
ValueQType->getArrayElementTypeNoTypeQual()
->isIntegralOrEnumerationType())) {
SmallString<10> StrBuffer;
for (unsigned I = 0; I < Value.getArraySize(); ++I) {
for (unsigned I = 0; I < Value.getArrayInitializedElts(); ++I) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewers: this particular change was done to avoid assertion failure within APInt on the following test case:

llvm/clang/test/SemaSYCL/attr-add-ir-attributes.cpp

Line 58 in c02a98c

[[__sycl_detail__::add_ir_attributes_function("Attr1", CEStr)]] void FunctionCEVal2() {}

const APValue &ArrayElem = Value.getArrayInitializedElt(I);
char C = static_cast<char>(ArrayElem.getInt().getExtValue());
if (C == 0)
if (C == '\0')
break;
StrBuffer += C;
}
Expand Down Expand Up @@ -1859,7 +1884,7 @@ def SYCLAddIRAttrCommonMembers : SYCLAddIRAttrMemberCodeHolder<[{
SmallString<10> StrBuffer;
for (const auto *InitE : InitListE->inits()) {
const Expr *InitNoImpCastE = InitE->IgnoreParenImpCasts();
char C = 0;
char C = '\0';
if (const auto *CharacterVal =
dyn_cast<CharacterLiteral>(InitNoImpCastE))
C = static_cast<char>(CharacterVal->getValue());
Expand All @@ -1870,7 +1895,7 @@ def SYCLAddIRAttrCommonMembers : SYCLAddIRAttrMemberCodeHolder<[{
return std::nullopt;

// Null terminator will end the string reading.
if (C == 0)
if (C == '\0')
break;

StrBuffer += C;
Expand Down
26 changes: 26 additions & 0 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8712,6 +8712,32 @@ void Sema::AddSYCLAddIRAttributesFunctionAttr(Decl *D,
CI))
return;
D->addAttr(Attr);

// There are compile-time SYCL properties which we would like to turn into
// attributes to enable compiler diagnostics.
// At the moment the only such property is related to virtual functions and
// it is turned into sycl_device attribute. This is a tiny optimization to
// avoid deep dive into the attribute if we already know that a declaration
// is a device declaration. It may have to be removed later if/when we add
// handling of more compile-time properties here.
if (D->hasAttr<SYCLDeviceAttr>())
return;

// SYCL Headers use template magic to pass key=value pairs to the attribute
// and we should make sure that all template instantiations are done before
// accessing attribute arguments.
if (hasDependentExpr(Attr->args_begin(), Attr->args_size()))
return;

SmallVector<std::pair<std::string, std::string>, 4> Pairs =
Attr->getFilteredAttributeNameValuePairs(Context);

for (const auto &[Key, Value] : Pairs) {
if (Key == "indirectly-callable") {
D->addAttr(SYCLDeviceAttr::CreateImplicit(Context));
break;
}
}
}

static void handleSYCLAddIRAttributesFunctionAttr(Sema &S, Decl *D,
Expand Down
96 changes: 96 additions & 0 deletions clang/test/AST/ast-attr-add-ir-attribute-extra-handling.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
// RUN: %clang_cc1 -fsycl-is-device -std=gnu++11 -ast-dump %s | FileCheck %s

// add_ir_attributes_function attribute used to represent compile-time SYCL
// properties and some of those properties are intended to be turned into
// attributes to enable various diagnostics.
//
// This test is intended to check one (and only, at least for now) of such
// tranformations: property with "indirectly-callable" key should have the same
// effect as applying sycl_device attribute and the test checks that we do add
// that attribute implicitly.

// CHECK-LABEL: ToBeTurnedIntoDeviceFunction 'void ()'
// CHECK: SYCLAddIRAttributesFunctionAttr
// CHECK: SYCLDeviceAttr {{.*}} Implicit
[[__sycl_detail__::add_ir_attributes_function("indirectly-callable", "void")]]
void ToBeTurnedIntoDeviceFunction();

// CHECK-LABEL: NotToBeTurnedIntoDeviceFunction 'void ()'
// CHECK-NOT: SYCLDeviceAttr {{.*}} Implicit
// CHECK: SYCLAddIRAttributesFunctionAttr
// CHECK-NOT: SYCLDeviceAttr {{.*}} Implicit
[[__sycl_detail__::add_ir_attributes_function("not-indirectly-callable", "void")]]
void NotToBeTurnedIntoDeviceFunction();

template <int V>
struct Metadata {
static constexpr const char *name = "not-indirectly-callable";
static constexpr const char *value = "void";
};

template <>
struct Metadata<42> {
static constexpr const char *name = "indirectly-callable";
static constexpr const char *value = "void";
};

// CHECK-LABEL: ToBeTurnedIntoDeviceFunctionAttrTemplateArgs 'void ()'
// CHECK: SYCLAddIRAttributesFunctionAttr
// CHECK: SYCLDeviceAttr {{.*}} Implicit
[[__sycl_detail__::add_ir_attributes_function(Metadata<42>::name, Metadata<42>::value)]]
void ToBeTurnedIntoDeviceFunctionAttrTemplateArgs();

// CHECK-LABEL: NotToBeTurnedIntoDeviceFunctionAttrTemplateArgs 'void ()'
// CHECK-NOT: SYCLDeviceAttr {{.*}} Implicit
// CHECK: SYCLAddIRAttributesFunctionAttr
// CHECK-NOT: SYCLDeviceAttr {{.*}} Implicit
[[__sycl_detail__::add_ir_attributes_function(Metadata<1>::name, Metadata<1>::value)]]
void NotToBeTurnedIntoDeviceFunctionAttrTemplateArgs();

// CHECK-LABEL: class Base definition
class Base {
// CHECK-LABEL: ToBeTurnedIntoDeviceFunctionAttrTemplateArgs 'void ()'
// CHECK: SYCLAddIRAttributesFunctionAttr
// CHECK: SYCLDeviceAttr {{.*}} Implicit
[[__sycl_detail__::add_ir_attributes_function(Metadata<42>::name, Metadata<42>::value)]]
virtual void ToBeTurnedIntoDeviceFunctionAttrTemplateArgs();

// CHECK-LABEL: NotToBeTurnedIntoDeviceFunctionAttrTemplateArgs 'void ()'
// CHECK-NOT: SYCLDeviceAttr {{.*}} Implicit
// CHECK: SYCLAddIRAttributesFunctionAttr
// CHECK-NOT: SYCLDeviceAttr {{.*}} Implicit
[[__sycl_detail__::add_ir_attributes_function(Metadata<1>::name, Metadata<1>::value)]]
virtual void NotToBeTurnedIntoDeviceFunctionAttrTemplateArgs();
};

// CHECK-LABEL: class Derived definition
class Derived : public Base {
// CHECK-LABEL: ToBeTurnedIntoDeviceFunctionAttrTemplateArgs 'void ()'
// CHECK: SYCLAddIRAttributesFunctionAttr
// CHECK: SYCLDeviceAttr {{.*}} Implicit
[[__sycl_detail__::add_ir_attributes_function(Metadata<42>::name, Metadata<42>::value)]]
void ToBeTurnedIntoDeviceFunctionAttrTemplateArgs() override;

// CHECK-LABEL: NotToBeTurnedIntoDeviceFunctionAttrTemplateArgs 'void ()'
// CHECK-NOT: SYCLDeviceAttr {{.*}} Implicit
// CHECK: SYCLAddIRAttributesFunctionAttr
// CHECK-NOT: SYCLDeviceAttr {{.*}} Implicit
[[__sycl_detail__::add_ir_attributes_function(Metadata<1>::name, Metadata<1>::value)]]
void NotToBeTurnedIntoDeviceFunctionAttrTemplateArgs() override;
};

// CHECK-LABEL: class SubDerived definition
class SubDerived : public Derived {
// CHECK-LABEL: ToBeTurnedIntoDeviceFunctionAttrTemplateArgs 'void ()'
// CHECK: SYCLAddIRAttributesFunctionAttr
// CHECK: SYCLDeviceAttr {{.*}} Implicit
[[__sycl_detail__::add_ir_attributes_function(Metadata<2>::name, Metadata<42>::name, Metadata<2>::value, Metadata<42>::value)]]
void ToBeTurnedIntoDeviceFunctionAttrTemplateArgs() override;

// CHECK-LABEL: NotToBeTurnedIntoDeviceFunctionAttrTemplateArgs 'void ()'
// CHECK-NOT: SYCLDeviceAttr {{.*}} Implicit
// CHECK: SYCLAddIRAttributesFunctionAttr
// CHECK-NOT: SYCLDeviceAttr {{.*}} Implicit
[[__sycl_detail__::add_ir_attributes_function(Metadata<1>::name, Metadata<2>::name, Metadata<1>::value, Metadata<2>::value)]]
void NotToBeTurnedIntoDeviceFunctionAttrTemplateArgs() override;
};
56 changes: 56 additions & 0 deletions clang/test/SemaSYCL/implicit-sycl-device-attr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// RUN: %clang_cc1 -fsycl-is-device -fcxx-exceptions -triple spir64 \
// RUN: -aux-triple x86_64-unknown-linux-gnu -Wno-return-type -verify \
// RUN: -Wno-sycl-2017-compat -fsyntax-only -std=c++17 %s

// add_ir_attributes_function attribute used to represent compile-time SYCL
// properties and some of those properties are intended to be turned into
// attributes to enable various diagnostics.
//
// "indirectly-callable" property is supposed to be turned into sycl_device
// attribute to make sure that functions market with that property are being
// diagnosed for violating SYCL device code restrictions.
//
// This test ensures that this is indeed the case.

namespace std {
class type_info; // needed to make typeid compile without corresponding include
} // namespace std

[[__sycl_detail__::add_ir_attributes_function("indirectly-callable", "void")]]
void cannot_use_recursion() {
// expected-error@+2 {{SYCL kernel cannot call a recursive function}}
// expected-note@-2 {{function implemented using recursion declared here}}
cannot_use_recursion();
}

[[__sycl_detail__::add_ir_attributes_function("indirectly-callable", "void")]]
void cannot_allocate_storage() {
new int; // expected-error {{SYCL kernel cannot allocate storage}}
}

[[__sycl_detail__::add_ir_attributes_function("indirectly-callable", "void")]]
void cannot_use_rtti() {
(void)typeid(int); // expected-error {{SYCL kernel cannot use rtti}}
}

[[__sycl_detail__::add_ir_attributes_function("indirectly-callable", "void")]]
void cannot_use_zero_length_array() {
// expected-error@+1 {{zero-length arrays are not permitted in SYCL device code}}
int mosterArr[0];
}

[[__sycl_detail__::add_ir_attributes_function("indirectly-callable", "void")]]
void cannot_use_long_double() {
// expected-error@+1 {{'long double' is not supported on this target}}
long double terrorLD;
}

[[__sycl_detail__::add_ir_attributes_function("indirectly-callable", "void")]]
void cannot_use_exceptions() {
try { // expected-error {{SYCL kernel cannot use exceptions}}
;
} catch (...) {
;
}
throw 20; // expected-error {{SYCL kernel cannot use exceptions}}
}