Skip to content

[SYCL] Fix FE attribute handling in SYCL_EXTERNAL functions. #1778

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 2 commits into from
Jun 8, 2020
Merged

[SYCL] Fix FE attribute handling in SYCL_EXTERNAL functions. #1778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2e625b
[SYCL] Fix FE attribute handling in SYCL_EXTERNAL functions.
kbobrovs May 29, 2020
2cda43a
[SQUASH] Review comments
kbobrovs May 30, 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
2 changes: 1 addition & 1 deletion clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10815,7 +10815,7 @@ def err_sycl_non_trivially_copy_ctor_dtor_type
def err_sycl_non_std_layout_type : Error<
"kernel parameter has non-standard layout class/struct type %0">;
def err_conflicting_sycl_kernel_attributes : Error<
"conflicting attributes applied to a SYCL kernel">;
"conflicting attributes applied to a SYCL kernel or SYCL_EXTERNAL function">;
def err_conflicting_sycl_function_attributes : Error<
"%0 attribute conflicts with '%1' attribute">;
def err_sycl_x_y_z_arguments_must_be_one : Error<
Expand Down
6 changes: 3 additions & 3 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12556,7 +12556,7 @@ class Sema final {
private:
// We store SYCL Kernels here and handle separately -- which is a hack.
// FIXME: It would be best to refactor this.
SmallVector<Decl*, 4> SyclDeviceDecls;
llvm::SetVector<Decl *> SyclDeviceDecls;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make this change? Switching to a setVector doesn't seem to help anything here, particularly when we are using it like a vector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SetVector allows both deduplication and deterministic iteration order. The former is needed because SYCL_EXTERNAL functions can also be called from kernels, in which case addSyclDeviceDecl(FD) on vector would lead to duplicates. The latter is needed for deterministic compiler behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying. Can you elaborate the problem better in the pull request message? I still don't have a good hold of the problem you're trying to solve.

Copy link
Contributor Author

@kbobrovs kbobrovs May 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It is all in the test case I added:

[[cl::intel_reqd_sub_group_size(2)]] void sg_size2() {}
[[cl::intel_reqd_sub_group_size(4)]] __attribute__((sycl_device)) void sg_size4() {
  sg_size2();
}

SYCL_EXTERNAL function with reqd_sub_group_size 4 calls a function with reqd_sub_group_size 2. This shoud be an error. Existing implementation detects it only when there is a kernel in place of SYCL_EXTERNAL function, and silently generates incorrect code for SYCL_EXTERNAL.

Copy link
Contributor Author

@kbobrovs kbobrovs May 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

SYCL_EXTERNAL == __attribute__((sycl_device)), BTW

// SYCL integration header instance for current compilation unit this Sema
// is associated with.
std::unique_ptr<SYCLIntegrationHeader> SyclIntHeader;
Expand All @@ -12567,8 +12567,8 @@ class Sema final {
bool ConstructingOpenCLKernel = false;

public:
void addSyclDeviceDecl(Decl *d) { SyclDeviceDecls.push_back(d); }
SmallVectorImpl<Decl *> &syclDeviceDecls() { return SyclDeviceDecls; }
void addSyclDeviceDecl(Decl *d) { SyclDeviceDecls.insert(d); }
llvm::SetVector<Decl *> &syclDeviceDecls() { return SyclDeviceDecls; }

/// Lazily creates and returns SYCL integration header instance.
SYCLIntegrationHeader &getSyclIntegrationHeader() {
Expand Down
9 changes: 9 additions & 0 deletions clang/lib/Sema/SemaSYCL.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1460,6 +1460,15 @@ void Sema::MarkDevice(void) {
// it is recursive.
MarkDeviceFunction Marker(*this);
Marker.SYCLCG.addToCallGraph(getASTContext().getTranslationUnitDecl());

// Iterate through SYCL_EXTERNAL functions and add them to the device decls.
for (const auto &entry : *Marker.SYCLCG.getRoot()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, @Fznamznon pointed out that this won't add class member functions, which are marked as SYCL_EXTERNAL into list of device functions which needs to be checked.
This is not critical, I guess, but I think deserves double-checking and probably a TODO, that attribute handling should be improved for class member functions as well

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't sure about it, so I double checked it locally, seems everything is fine with class members, but we could add a corresponding test in next patches.

if (auto *FD = dyn_cast<FunctionDecl>(entry.Callee->getDecl())) {
if (FD->hasAttr<SYCLDeviceAttr>() && !FD->hasAttr<SYCLKernelAttr>())
addSyclDeviceDecl(FD);
}
}

for (Decl *D : syclDeviceDecls()) {
if (auto SYCLKernel = dyn_cast<FunctionDecl>(D)) {
llvm::SmallPtrSet<FunctionDecl *, 10> VisitedSet;
Expand Down
11 changes: 11 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 @@ -48,6 +48,17 @@ void bar() {
kernel<class kernel_name5>([]() [[cl::intel_reqd_sub_group_size(2)]] { });
}

#ifdef TRIGGER_ERROR
// expected-note@+1 {{conflicting attribute is here}}
[[cl::intel_reqd_sub_group_size(2)]] void sg_size2() {}

// expected-note@+2 {{conflicting attribute is here}}
// expected-error@+1 {{conflicting attributes applied to a SYCL kernel}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This diagnostic doesn't seem to be correct, because function it is pointing to is not a SYCL kernel

Copy link
Contributor

Choose a reason for hiding this comment

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

This diagnostic seems strange as well, why does it think that sg_size4 and sg_size2 are the same kernel?

Should this conflict be diagnosed on a call expression instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This diagnostic doesn't seem to be correct, because function it is pointing to is not a SYCL kernel

OK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This diagnostic seems strange as well, why does it think that sg_size4 and sg_size2 are the same kernel?

Why do you think "it think that sg_size4 and sg_size2 are the same kernel" - please elaborate.

Should this conflict be diagnosed on a call expression instead?

This is a question to existing design. I'm not changing anything in the diagnostics mechanism. I'm fixing existing bug where SYCL_EXTERNAL functions were missing in that diagnostics - I add them.

Copy link
Contributor

Choose a reason for hiding this comment

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

From the error message, it says "conflicting attributes applied to a SYCL kernel". From the notes, it specifies attributes on two separate declarations.

So by reading the error + notes, it implies that there are two conflicting attributes on a single kernel, yet they are on different functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error message existed before - I did not invent it, I just used it for SYCL_EXTERNAL function. Do you suggest to fix prior problems?

Copy link
Contributor

Choose a reason for hiding this comment

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

We do this already? We have other conditions where we diagnose an error on a kernel about conflicting attributes, and then point to an attribute on another declaration?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. SYCL attributes are propagated from called device function up to the kernel (AFAIK it is spec defined behavior which I find pretty magic), sometimes there are several functions called from kernel and their attribute values can conflict. Diagnosing happens here

llvm/clang/lib/Sema/SemaSYCL.cpp

Line 1484 in 12d14e8

Diag(SYCLKernel->getLocation(),

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, well, the way we report that is awful. But not needing to be fixed in this patch. Thanks @Fznamznon. The error-message should likely be changed to be more clear about this if anyone ends up getting to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// expected-error@+1 {{conflicting attributes applied to a SYCL kernel}}
// expected-error@+1 {{conflicting attributes applied to a SYCL kernel or SYCL_EXTERNAL function}}

[[cl::intel_reqd_sub_group_size(4)]] __attribute__((sycl_device)) void sg_size4() {
sg_size2();
}
#endif

// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name1
// CHECK: IntelReqdSubGroupSizeAttr {{.*}} 16
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name2
Expand Down