Skip to content

[SYCL] Add support for [[intel::reqd_sub_group_… #2137

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 10 commits into from
Jul 22, 2020
Merged

[SYCL] Add support for [[intel::reqd_sub_group_… #2137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1554ae7
[WIP] [DO NOT REVIEW] [SYCL] Add support for [[intel::reqd_sub_group_…
smanna12 Jul 20, 2020
a655790
Fix Clang-format issues
smanna12 Jul 20, 2020
65b3634
Update diagnostic message
smanna12 Jul 21, 2020
f3cdca1
Add note
smanna12 Jul 21, 2020
9eb186f
update note based on review comments
smanna12 Jul 21, 2020
804f147
Fix clang-format issue
smanna12 Jul 21, 2020
8d2c340
Update all tests with new attribute spelling
smanna12 Jul 21, 2020
07aa988
Update test and fix clang-format error
smanna12 Jul 21, 2020
59dde52
Fix clang-format error
smanna12 Jul 21, 2020
7c4204e
Correct Doc
smanna12 Jul 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
4 changes: 3 additions & 1 deletion clang/include/clang/Basic/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1288,7 +1288,9 @@ def LoopUnrollHint : InheritableAttr {
}

def IntelReqdSubGroupSize: InheritableAttr {
let Spellings = [GNU<"intel_reqd_sub_group_size">, CXX11<"cl", "intel_reqd_sub_group_size">];
let Spellings = [GNU<"intel_reqd_sub_group_size">,
CXX11<"cl", "intel_reqd_sub_group_size">,
CXX11<"intel", "reqd_sub_group_size">];
let Args = [ExprArgument<"SubGroupSize">];
let Subjects = SubjectList<[Function, CXXMethod], ErrorDiag>;
let Documentation = [IntelReqdSubGroupSizeDocs];
Expand Down
20 changes: 17 additions & 3 deletions clang/include/clang/Basic/AttrDocs.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3472,9 +3472,10 @@ code. See `cl_intel_required_subgroup_size
for details.

SYCL documentation:
The [[cl::intel_reqd_sub_group_size(n)]] attribute indicates that the kernel
must be compiled and executed with a sub-group of size n. The value of n must be
set to a sub-group size supported by the device, or device compilation will fail.
The [[cl::intel_reqd_sub_group_size(n)]] and [[intel::reqd_sub_group_size(n)]]
attribute indicates that the kernel must be compiled and executed with a
sub-group of size n. The value of n must be set to a sub-group size supported
by the device, or device compilation will fail.

In addition to device functions, the required sub-group size attribute may also
be specified in the definition of a named functor object and lambda functions,
Expand All @@ -3495,6 +3496,19 @@ as in the examples below:
/* kernel code */
});

class Functor
{
[[intel::reqd_sub_group_size(16)]] void operator()(item<1> item)
{
/* kernel code */
}
}

kernel<class kernel_name>(
[]() [[intel::reqd_sub_group_size(n)]] {
/* kernel code */
});

See Sub-groups for NDRange Parallelism proposal in sycl/doc/extensions/sub_group_ndrange/sub_group_ndrange.md
}];
}
Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10969,6 +10969,11 @@ def err_ivdep_declrefexpr_arg : Error<
def warn_ivdep_redundant : Warning <"ignoring redundant Intel FPGA loop "
"attribute 'ivdep': safelen %select{INF|%1}0 >= safelen %select{INF|%3}2">,
InGroup<IgnoredAttributes>;
def warn_attribute_spelling_deprecated : Warning<
"attribute %0 is deprecated">,
InGroup<DeprecatedAttributes>;
def note_spelling_suggestion : Note<
"did you mean to use %0 instead?">;

// errors of expect.with.probability
def err_probability_not_constant_float : Error<
Expand Down
7 changes: 7 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 @@ -3014,6 +3014,13 @@ static void handleSubGroupSize(Sema &S, Decl *D, const ParsedAttr &AL) {
if (D->getAttr<IntelReqdSubGroupSizeAttr>())
S.Diag(AL.getLoc(), diag::warn_duplicate_attribute) << AL;

if (AL.getAttributeSpellingListIndex() ==
IntelReqdSubGroupSizeAttr::CXX11_cl_intel_reqd_sub_group_size) {
S.Diag(AL.getLoc(), diag::warn_attribute_spelling_deprecated) << AL;
S.Diag(AL.getLoc(), diag::note_spelling_suggestion)
<< "'intel::reqd_sub_group_size'";
}

S.addIntelReqdSubGroupSizeAttr(D, AL, E);
}

Expand Down
40 changes: 30 additions & 10 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
@@ -1,22 +1,29 @@
// RUN: %clang_cc1 -fsycl -fsycl-is-device -fsyntax-only -verify -DTRIGGER_ERROR %s
// RUN: %clang_cc1 -fsycl -fsycl-is-device -ast-dump %s | FileCheck %s

[[cl::intel_reqd_sub_group_size(4)]] void foo() {} // expected-note {{conflicting attribute is here}}
[[intel::reqd_sub_group_size(4)]] void foo() {} // expected-note {{conflicting attribute is here}}
// expected-note@-1 {{conflicting attribute is here}}
[[cl::intel_reqd_sub_group_size(32)]] void baz() {} // expected-note {{conflicting attribute is here}}
[[intel::reqd_sub_group_size(32)]] void baz() {} // expected-note {{conflicting attribute is here}}

class Functor16 {
public:
// expected-warning@+2 {{attribute 'intel_reqd_sub_group_size' is deprecated}}
// expected-note@+1 {{did you mean to use 'intel::reqd_sub_group_size' instead?}}
[[cl::intel_reqd_sub_group_size(16)]] void operator()() {}
};

class Functor8 { // expected-error {{conflicting attributes applied to a SYCL kernel}}
public:
[[cl::intel_reqd_sub_group_size(8)]] void operator()() { // expected-note {{conflicting attribute is here}}
[[intel::reqd_sub_group_size(8)]] void operator()() { // expected-note {{conflicting attribute is here}}
foo();
}
};

class Functor4 {
public:
[[intel::reqd_sub_group_size(12)]] void operator()() {}
};

class Functor {
public:
void operator()() {
Expand Down Expand Up @@ -46,24 +53,31 @@ void bar() {
});
#endif

kernel<class kernel_name5>([]() [[cl::intel_reqd_sub_group_size(2)]] { });
kernel<class kernel_name6>([]() [[cl::intel_reqd_sub_group_size(4)]] { foo(); });
kernel<class kernel_name5>([]() [[intel::reqd_sub_group_size(2)]]{});
kernel<class kernel_name6>([]() [[intel::reqd_sub_group_size(4)]] { foo(); });
// expected-warning@+2 {{attribute 'intel_reqd_sub_group_size' is deprecated}}
// expected-note@+1 {{did you mean to use 'intel::reqd_sub_group_size' instead?}}
kernel<class kernel_name7>([]() [[cl::intel_reqd_sub_group_size(6)]]{});

Functor4 f4;
kernel<class kernel_name8>(f4);
}

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

[[intel::reqd_sub_group_size(16)]] SYCL_EXTERNAL void B() {
A();
}

#ifdef TRIGGER_ERROR
// expected-note@+1 {{conflicting attribute is here}}
[[cl::intel_reqd_sub_group_size(2)]] void sg_size2() {}
[[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}}
[[cl::intel_reqd_sub_group_size(4)]] __attribute__((sycl_device)) void sg_size4() {
[[intel::reqd_sub_group_size(4)]] __attribute__((sycl_device)) void sg_size4() {
sg_size2();
}
#endif
Expand All @@ -77,3 +91,9 @@ void bar() {
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name5
// CHECK: IntelReqdSubGroupSizeAttr {{.*}}
// CHECK-NEXT: IntegerLiteral{{.*}}2{{$}}
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name7
// CHECK: IntelReqdSubGroupSizeAttr {{.*}}
// CHECK-NEXT: IntegerLiteral{{.*}}6{{$}}
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name8
// CHECK: IntelReqdSubGroupSizeAttr {{.*}}
// CHECK-NEXT: IntegerLiteral{{.*}}12{{$}}
6 changes: 3 additions & 3 deletions clang/test/SemaSYCL/sycl-device-reqd-sub-group-size-template.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
// RUN: %clang_cc1 -fsycl -fsycl-is-device -fsyntax-only -ast-dump -verify -pedantic %s | FileCheck %s

// Test that checkes template parameter support for 'intel_reqd_sub_group_size' attribute on sycl device.
// Test that checkes template parameter support for 'reqd_sub_group_size' attribute on sycl device.

template <int SIZE>
class KernelFunctor {
public:
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels weird to me that existing tests all throw warnings now. I am not sure what is usually done when spellings are changed though. Personally I would just prefer updating these tests to use the new spelling (except where this functionality is actually being checked). @erichkeane thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I could go either way. There aren't that many tests to just 'fix' the spelling of here, so I'd probably suggest just fixing all the spellings instead.

If this was more widespread (that is, the tests were a pain to change), or a situation where there was a difference in the implementation, I'd consider just using the -Wno flag in the tests. In this case, I think just changing the spelling in the tests is better than adding the warning/note to ALL of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, probably leave at least 1 to validate the deprecated warning :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea. That's what I meant by 'except where this functionality is actually being checked' :)

@smanna12 sorry but can you make this change? I think it's just cleaner this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea. That's what I meant by 'except where this functionality is actually being checked' :)

@smanna12 sorry but can you make this change? I think it's just cleaner this way.

Sure. I will do this.

Copy link
Contributor

@erichkeane erichkeane Jul 21, 2020 •
edited by smanna12
Loading

Choose a reason for hiding this comment

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

I think you still need to leave ONE example (perhaps add a new example) to validate the new warning/note.

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, right. By mistake, I changed all spellings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the multiple submission.

//expected-error@+1{{'intel_reqd_sub_group_size' attribute requires a positive integral compile time constant expression}}
[[cl::intel_reqd_sub_group_size(SIZE)]] void operator()() {}
// expected-error@+1{{'reqd_sub_group_size' attribute requires a positive integral compile time constant expression}}
[[intel::reqd_sub_group_size(SIZE)]] void operator()() {}
};

int main() {
Expand Down
2 changes: 1 addition & 1 deletion clang/test/SemaSYCL/sycl-esimd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ void kernel0(F f) __attribute__((sycl_kernel)) {
}

// expected-note@+1{{conflicting attribute is here}}
[[cl::intel_reqd_sub_group_size(2)]] void g0() {}
[[intel::reqd_sub_group_size(2)]] void g0() {}

void test0() {
// expected-error@+2{{conflicting attributes applied to a SYCL kernel}}
Expand Down