Skip to content

[SYCL] reqd_work_group_size attribute is reversed #1234

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
romanovvlad merged 17 commits into from
Mar 10, 2020
Merged

[SYCL] reqd_work_group_size attribute is reversed #1234

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2fc939b
[SYCL] reqd_work_group_size attribute is reversed
fadeeval Feb 28, 2020
3582e97
[SYCL] reqd_work_group_size attribute is reversed (fix #2)
fadeeval Feb 28, 2020
3c6ea83
[SYCL] reqd_work_group_size attribute is reversed (fixed #3)
fadeeval Feb 28, 2020
b14013d
[SYCL] reqd_work_group_size attribute is reversed (fix #4)
fadeeval Feb 28, 2020
1d0d751
[SYCL] reqd_work_group_size attribute is reversed (fix #5)
fadeeval Mar 2, 2020
ff6746a
[SYCL] reqd_work_group_size attribute is reversed (fix #6)
fadeeval Mar 3, 2020
245f323
[SYCL] reqd_work_group_size attribute is reversed (fix #6)
fadeeval Mar 3, 2020
9125bc1
[SYCL] reqd_work_group_size attribute is reversed (fix #8)
fadeeval Mar 3, 2020
a7eef4b
[SYCL] reqd_work_group_size attribute is reversed (fix #9)
fadeeval Mar 4, 2020
2b6df01
[SYCL] reqd_work_group_size attribute is reversed (fix #10)
fadeeval Mar 4, 2020
5c3501f
[SYCL] reqd_work_group_size attribute is reversed (fix #11)
fadeeval Mar 4, 2020
43f265f
[SYCL] reqd_work_group_size attribute is reversed (fix #12)
fadeeval Mar 4, 2020
5831eb4
[SYCL] reqd_work_group_size attribute is reversed (fix #13)
fadeeval Mar 5, 2020
8696f6e
[SYCL] reqd_work_group_size attribute is reversed (fix #13)
fadeeval Mar 5, 2020
dc704bf
[SYCL] reqd_work_group_size attribute is reversed (fix #14)
fadeeval Mar 6, 2020
9ab8cf7
[SYCL] reqd_work_group_size attribute is reversed
fadeeval Mar 6, 2020
abe6d2d
[SYCL] reqd_work_group_size attribute is reversed (fix #15)
fadeeval Mar 10, 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/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1147,7 +1147,7 @@ def SYCLIntelKernelArgsRestrict : InheritableAttr {

def SYCLIntelNumSimdWorkItems : InheritableAttr {
let Spellings = [CXX11<"intelfpga","num_simd_work_items">];
let Args = [UnsignedArgument<"Number">];
let Args = [IntArgument<"Number">];
Copy link
Contributor

@asavonic asavonic Mar 6, 2020

Choose a reason for hiding this comment

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

I guess this should be removed.

Copy link
Contributor Author

@fadeeval fadeeval Mar 10, 2020

Choose a reason for hiding this comment

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

Yes.

let LangOpts = [SYCLIsDevice, SYCLIsHost];
let Subjects = SubjectList<[Function], ErrorDiag>;
let Documentation = [SYCLIntelNumSimdWorkItemsAttrDocs];
Expand Down
17 changes: 10 additions & 7 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2895,7 +2895,7 @@ static bool checkWorkGroupSizeValues(Sema &S, Decl *D, const ParsedAttr &Attr,
if (const auto *A = D->getAttr<SYCLIntelMaxGlobalWorkDimAttr>())
if (A->getNumber() == 0)
Result &= checkZeroDim(A, WGSize[0], WGSize[1], WGSize[2],
/*ReverseAttrs=*/ true);
/*ReverseAttrs=*/true);

if (const auto *A = D->getAttr<SYCLIntelMaxWorkGroupSizeAttr>()) {
if (!(WGSize[0] <= A->getXDim() && WGSize[1] <= A->getYDim() &&
Expand Down Expand Up @@ -2935,14 +2935,17 @@ static void handleWorkGroupSize(Sema &S, Decl *D, const ParsedAttr &AL) {
}
}

if (!checkWorkGroupSizeValues(S, D, AL, WGSize))
return;

// For a SYCLDevice WorkGroupAttr arguments are reversed
if (S.getLangOpts().SYCLIsDevice) {
std::swap(WGSize[0], WGSize[2]);
}
WorkGroupAttr *Existing = D->getAttr<WorkGroupAttr>();
if (Existing && !(Existing->getXDim() == WGSize[0] &&
Existing->getYDim() == WGSize[1] &&
Existing->getZDim() == WGSize[2]))
if (Existing &&
!(Existing->getXDim() == WGSize[0] && Existing->getYDim() == WGSize[1] &&
Copy link
Contributor

@asavonic asavonic Mar 3, 2020

Choose a reason for hiding this comment

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

This check is not reversed. Does the warning below still work correctly?

Copy link
Contributor Author

@fadeeval fadeeval Mar 3, 2020

Choose a reason for hiding this comment

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

I think it can work. Is there test for it?

Copy link
Contributor Author

@fadeeval fadeeval Mar 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If yes, then it works fine.

Copy link
Contributor

@asavonic asavonic Mar 3, 2020

Choose a reason for hiding this comment

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

I think it can work. Is there test for it?

That is what I was going to ask.

If I'm reading this code correctly, we compare XDim of an existing attribute with WGSize[0], which is going to be ZDim. Shouldn't we compare existing XDim with WGSize[2] instead?

Copy link
Contributor Author

@fadeeval fadeeval Mar 3, 2020

Choose a reason for hiding this comment

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

Maybe you are right

Copy link
Contributor Author

@fadeeval fadeeval Mar 3, 2020

Choose a reason for hiding this comment

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

I will try to prove it.

Copy link
Contributor Author

@fadeeval fadeeval Mar 4, 2020

Choose a reason for hiding this comment

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

Yes, it need to be reversed.

Copy link
Contributor

@asavonic asavonic Mar 4, 2020

Choose a reason for hiding this comment

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

I suspect that the diagnostic was not tested. Can you add a test for it?

Copy link
Contributor

@MrSidims MrSidims Mar 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

A general case was tested, see: reqd-work-group-size.cpp:23 - 31, and these diagnostics are firing anyway.
But with this patch there is a corner case introduced, that was untested, like:

[[cl::reqd_work_group_size(4, 4, 1)]]
[[cl::reqd_work_group_size(1, 4, 4)]]
void foo() { /*...*/ }

Copy link
Contributor Author

@fadeeval fadeeval Mar 4, 2020

Choose a reason for hiding this comment

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

I add one test, that prove rightness of the implementation.

Existing->getZDim() == WGSize[2]))
S.Diag(AL.getLoc(), diag::warn_duplicate_attribute) << AL;
if (!checkWorkGroupSizeValues(S, D, AL, WGSize))
return;

D->addAttr(::new (S.Context)
WorkGroupAttr(S.Context, AL, WGSize[0], WGSize[1], WGSize[2]));
Expand Down
7 changes: 3 additions & 4 deletions clang/test/CodeGenSYCL/reqd-work-group-size.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,12 @@ void bar() {
kernel<class kernel_name2>(f);

kernel<class kernel_name3>(
[]() [[cl::reqd_work_group_size(8, 8, 8)]] {});
[]() [[cl::reqd_work_group_size(8, 8, 8)]]{});
}

// CHECK: define spir_kernel void @{{.*}}kernel_name1() {{.*}} !reqd_work_group_size ![[WGSIZE32:[0-9]+]]
// CHECK: define spir_kernel void @{{.*}}kernel_name2() {{.*}} !reqd_work_group_size ![[WGSIZE8:[0-9]+]]
// CHECK: define spir_kernel void @{{.*}}kernel_name3() {{.*}} !reqd_work_group_size ![[WGSIZE88:[0-9]+]]
// CHECK: ![[WGSIZE32]] = !{i32 32, i32 16, i32 16}
// CHECK: ![[WGSIZE8]] = !{i32 8, i32 1, i32 1}
// CHECK: ![[WGSIZE32]] = !{i32 16, i32 16, i32 32}
// CHECK: ![[WGSIZE8]] = !{i32 1, i32 1, i32 8}
// CHECK: ![[WGSIZE88]] = !{i32 8, i32 8, i32 8}

2 changes: 1 addition & 1 deletion clang/test/CodeGenSYCL/sycl-multi-kernel-attr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@ void bar() {
}

// CHECK: define spir_kernel void @{{.*}}kernel_name() {{.*}} !reqd_work_group_size ![[WGSIZE:[0-9]+]] !intel_reqd_sub_group_size ![[SGSIZE:[0-9]+]]
// CHECK: ![[WGSIZE]] = !{i32 32, i32 16, i32 16}
// CHECK: ![[WGSIZE]] = !{i32 16, i32 16, i32 32}
// CHECK: ![[SGSIZE]] = !{i32 4}
4 changes: 2 additions & 2 deletions clang/test/SemaSYCL/intel-max-global-work-dim.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ int main() {
kernel<class test_kernel5>(
TRIFuncObjGood2());
// CHECK-LABEL: FunctionDecl {{.*}} _ZTSZ4mainE12test_kernel5
// CHECK: ReqdWorkGroupSizeAttr {{.*}} 4 1 1
// CHECK: SYCLIntelMaxWorkGroupSizeAttr {{.*}} 8 1 1
// CHECK: ReqdWorkGroupSizeAttr {{.*}} 1 1 4
// CHECK: SYCLIntelMaxWorkGroupSizeAttr {{.*}} 1 1 8
// CHECK: SYCLIntelMaxGlobalWorkDimAttr {{.*}} 3

#ifdef TRIGGER_ERROR
Expand Down
31 changes: 20 additions & 11 deletions clang/test/SemaSYCL/reqd-work-group-size.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
class Functor {
public:
[[cl::reqd_work_group_size(4, 1, 1)]] void operator()() {}

};

template <typename name, typename Func>
Expand All @@ -24,25 +23,33 @@ void bar() {
// expected-note@-1 {{conflicting attribute is here}}
[[cl::reqd_work_group_size(32, 1, 1)]] void f32x1x1() {} // expected-note {{conflicting attribute is here}}

[[cl::reqd_work_group_size(16, 1, 1)]] void f16x1x1() {} // expected-note {{conflicting attribute is here}}
[[cl::reqd_work_group_size(16, 1, 1)]] void f16x1x1() {} // expected-note {{conflicting attribute is here}}
[[cl::reqd_work_group_size(16, 16, 1)]] void f16x16x1() {} // expected-note {{conflicting attribute is here}}

[[cl::reqd_work_group_size(32, 32, 1)]] void f32x32x1() {} // expected-note {{conflicting attribute is here}}
[[cl::reqd_work_group_size(32, 32, 1)]] void f32x32x1() {} // expected-note {{conflicting attribute is here}}
[[cl::reqd_work_group_size(32, 32, 32)]] void f32x32x32() {} // expected-note {{conflicting attribute is here}}

class Functor16 {
public:
[[cl::reqd_work_group_size(16, 1, 1)]] void operator()() {}
[[cl::reqd_work_group_size(16, 1, 1)]] [[cl::reqd_work_group_size(16, 1, 1)]] void operator()() {}
Comment on lines -35 to +34
Copy link
Contributor Author

@fadeeval fadeeval Mar 4, 2020

Choose a reason for hiding this comment

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

This is a new one.

};

#ifdef TRIGGER_ERROR
class Functor32 {
public:
//expected-warning@+2{{attribute 'reqd_work_group_size' is already applied with different parameters}}
// expected-error@+1{{'reqd_work_group_size' attribute conflicts with 'reqd_work_group_size' attribute}}
[[cl::reqd_work_group_size(32, 1, 1)]] [[cl::reqd_work_group_size(1, 1, 32)]] void operator()() {}
};
#endif
class Functor16x16x16 {
public:
[[cl::reqd_work_group_size(16, 16, 16)]] void operator()() {}
};

class Functor8 { // expected-error {{conflicting attributes applied to a SYCL kernel}}
public:
[[cl::reqd_work_group_size(8, 1, 1)]] void operator()() { // expected-note {{conflicting attribute is here}}
[[cl::reqd_work_group_size(1, 1, 8)]] void operator()() { // expected-note {{conflicting attribute is here}}
f4x1x1();
}
};
Expand Down Expand Up @@ -77,15 +84,17 @@ void bar() {
FunctorAttr fattr;
kernel<class kernel_name4>(fattr);

kernel<class kernel_name5>([]() [[cl::reqd_work_group_size(32, 32, 32)]] {
f32x32x32();
kernel<class kernel_name5>([]() [[cl::reqd_work_group_size(32, 32, 32), cl::reqd_work_group_size(32, 32, 32)]] {
f32x32x32();
});


#ifdef TRIGGER_ERROR
Functor8 f8;
kernel<class kernel_name6>(f8);

Functor32 f32;
kernel<class kernel_name1>(f32);

kernel<class kernel_name7>([]() { // expected-error {{conflicting attributes applied to a SYCL kernel}}
f4x1x1();
f32x1x1();
Expand All @@ -102,17 +111,17 @@ void bar() {
});

// expected-error@+1 {{expected variable name or 'this' in lambda capture list}}
kernel<class kernel_name10>([[cl::reqd_work_group_size(32, 32, 32)]] []() {
kernel<class kernel_name10>([[cl::reqd_work_group_size(32, 32, 32)]][]() {
f32x32x32();
});

#endif
}

// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name1
// CHECK: ReqdWorkGroupSizeAttr {{.*}} 16 1 1
// CHECK: ReqdWorkGroupSizeAttr {{.*}} 1 1 16
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name2
// CHECK: ReqdWorkGroupSizeAttr {{.*}} 4 1 1
// CHECK: ReqdWorkGroupSizeAttr {{.*}} 1 1 4
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name3
// CHECK: ReqdWorkGroupSizeAttr {{.*}} 16 16 16
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name4
Expand Down