Skip to content

[SYCL] Fix check for reqd_sub_group_size attribute mismatches #1905

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 6 commits into from
Jun 23, 2020
Merged

[SYCL] Fix check for reqd_sub_group_size attribute mismatches #1905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8a35cf0
[SYCL] Fix check for reqd_sub_group_size attribute mismatches
AlexeySachkov Jun 16, 2020
608ee3a
[SYCL] Introduce IntegerExprArgument
AlexeySachkov Jun 17, 2020
6b578a6
Fix formatting issue
AlexeySachkov Jun 19, 2020
3bee3df
[SYCL] Revert all improvements, just fix a bug
AlexeySachkov Jun 19, 2020
540853f
[SYCL] Revert change of getIntExprValue return type
AlexeySachkov Jun 22, 2020
c625a69
Update clang/lib/Sema/SemaSYCL.cpp
AlexeySachkov Jun 22, 2020
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
14 changes: 8 additions & 6 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,11 +308,12 @@ static void reportConflictingAttrs(Sema &S, FunctionDecl *F, const Attr *A1,
F->setInvalidDecl();
}

// Returns the signed constant integer value represented by given expression.
static int64_t getIntExprValue(Sema &S, const Expr *E) {
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 decided to replace this function with extension to IntelReqdSubGroupSizeAttr class to add caching functionality to avoid re-evaluating argument expression each time we need the value, but I'm not entirely sure that this is a correct approach or I implemented it correctly - please advise here

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling, that I've already seen that type of function is Sema, like 'check unsigned int arguments' (can't grep it right now), so I see no value in the function above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use this function then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we use this function then?

I prefer to have some cached value instead of evaluating attribute argument each time we need it to perform some correctness check

/// Returns the signed constant integer value represented by given expression
static int64_t getIntExprValue(const Expr *E, ASTContext &Ctx) {
llvm::APSInt Val(32);
bool IsValid = E->isIntegerConstantExpr(Val, S.getASTContext());
bool IsValid = E->isIntegerConstantExpr(Val, Ctx);
assert(IsValid && "expression must be constant integer");
(void)IsValid;
return Val.getSExtValue();
}

Expand Down Expand Up @@ -1696,15 +1697,16 @@ void Sema::MarkDevice(void) {
KernelBody ? KernelBody->getAttr<SYCLSimdAttr>() : nullptr;
if (auto *Existing =
SYCLKernel->getAttr<IntelReqdSubGroupSizeAttr>()) {
if (Existing->getSubGroupSize() != Attr->getSubGroupSize()) {
if (getIntExprValue(Existing->getSubGroupSize(), getASTContext()) !=
getIntExprValue(Attr->getSubGroupSize(), getASTContext())) {
Diag(SYCLKernel->getLocation(),
diag::err_conflicting_sycl_kernel_attributes);
Diag(Existing->getLocation(), diag::note_conflicting_attribute);
Diag(Attr->getLocation(), diag::note_conflicting_attribute);
SYCLKernel->setInvalidDecl();
}
} else if (KBSimdAttr &&
(getIntExprValue(*this, Attr->getSubGroupSize()) != 1)) {
} else if (KBSimdAttr && (getIntExprValue(Attr->getSubGroupSize(),
getASTContext()) != 1)) {
reportConflictingAttrs(*this, KernelBody, KBSimdAttr, Attr);
} else {
SYCLKernel->addAttr(A);
Expand Down
9 changes: 9 additions & 0 deletions clang/test/SemaSYCL/reqd-sub-group-size-device.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,16 @@ void bar() {
baz();
});
#endif

kernel<class kernel_name5>([]() [[cl::intel_reqd_sub_group_size(2)]] { });
kernel<class kernel_name6>([]() [[cl::intel_reqd_sub_group_size(4)]] { foo(); });
}

[[cl::intel_reqd_sub_group_size(16)]] SYCL_EXTERNAL void B();
[[cl::intel_reqd_sub_group_size(16)]] void A() {
}
[[cl::intel_reqd_sub_group_size(16)]] SYCL_EXTERNAL void B() {
A();
}

#ifdef TRIGGER_ERROR
Expand Down