Skip to content

Commit 7ef84b6

Browse files
smanna12smanna12
authored
[SYCL] [FPGA] Disable swapping dimension values of WorkGroupAttr arguments for semantic checks (#2962)
This is a followup comments on #2906 (comment) and #2906 (comment). Currently For a SYCLDevice WorkGroupAttr arguments are reversed. if (S.getLangOpts().SYCLIsDevice) std::swap(XDimExpr, ZDimExpr); There are few problems with this approach (swapping order in AST): 1. A user who writes the order as X, Y, Z in their source will get Z, Y, X when they pretty print the AST. 2. The same will happen when dumping the AST to text or JSON or generating diagnostic messages since the fist and third arguments of WorkGroupAttr are reversed. 3. This is very confusing to see the different order than the oroginal source codes. max_work_group_size() follows same rules as reqd_work_group_size() in Sema and LLVM IR generation. This patch keeps the arguments in the same order as they were parsed in doing semantic checks, and when performing code generation, we would change the argument order when lowering to LLVM IR and not when creating the semantic attribute. Signed-off-by: Soumi Manna <soumi.manna@intel.com>
1 parent 9112252 commit 7ef84b6

10 files changed

+51
-30
lines changed

‎clang/lib/CodeGen/CodeGenFunction.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -627,6 +627,11 @@ void CodeGenFunction::EmitOpenCLKernelMetadata(const FunctionDecl *FD,
627627
A->getYDim()->getIntegerConstantExpr(FD->getASTContext());
628628
Optional<llvm::APSInt> ZDimVal =
629629
A->getZDim()->getIntegerConstantExpr(FD->getASTContext());
630+
631+
// For a SYCLDevice ReqdWorkGroupSizeAttr arguments are reversed.
632+
if (getLangOpts().SYCLIsDevice)
633+
std::swap(XDimVal, ZDimVal);
634+
630635
llvm::Metadata *AttrMDArgs[] = {
631636
llvm::ConstantAsMetadata::get(
632637
Builder.getInt32(XDimVal->getZExtValue())),
@@ -702,6 +707,11 @@ void CodeGenFunction::EmitOpenCLKernelMetadata(const FunctionDecl *FD,
702707
A->getYDim()->getIntegerConstantExpr(FD->getASTContext());
703708
Optional<llvm::APSInt> ZDimVal =
704709
A->getZDim()->getIntegerConstantExpr(FD->getASTContext());
710+
711+
// For a SYCLDevice SYCLIntelMaxWorkGroupSizeAttr arguments are reversed.
712+
if (getLangOpts().SYCLIsDevice)
713+
std::swap(XDimVal, ZDimVal);
714+
705715
llvm::Metadata *AttrMDArgs[] = {
706716
llvm::ConstantAsMetadata::get(
707717
Builder.getInt32(XDimVal->getZExtValue())),

‎clang/lib/Sema/SemaDeclAttr.cpp

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3026,19 +3026,19 @@ static bool checkWorkGroupSizeValues(Sema &S, Decl *D, const ParsedAttr &AL,
30263026
A->getValue()->getIntegerConstantExpr(S.Context)->getSExtValue();
30273027
if (AttrValue == 0) {
30283028
Result &=
3029-
checkZeroDim(A, getExprValue(AL.getArgAsExpr(2), S.getASTContext()),
3029+
checkZeroDim(A, getExprValue(AL.getArgAsExpr(0), S.getASTContext()),
30303030
getExprValue(AL.getArgAsExpr(1), S.getASTContext()),
3031-
getExprValue(AL.getArgAsExpr(0), S.getASTContext()),
3031+
getExprValue(AL.getArgAsExpr(2), S.getASTContext()),
30323032
/*ReverseAttrs=*/true);
30333033
}
30343034
}
30353035

30363036
if (const auto *A = D->getAttr<SYCLIntelMaxWorkGroupSizeAttr>()) {
3037-
if (!((getExprValue(AL.getArgAsExpr(2), S.getASTContext()) <=
3037+
if (!((getExprValue(AL.getArgAsExpr(0), S.getASTContext()) <=
30383038
getExprValue(A->getXDim(), S.getASTContext())) &&
30393039
(getExprValue(AL.getArgAsExpr(1), S.getASTContext()) <=
30403040
getExprValue(A->getYDim(), S.getASTContext())) &&
3041-
(getExprValue(AL.getArgAsExpr(0), S.getASTContext()) <=
3041+
(getExprValue(AL.getArgAsExpr(2), S.getASTContext()) <=
30423042
getExprValue(A->getZDim(), S.getASTContext())))) {
30433043
S.Diag(AL.getLoc(), diag::err_conflicting_sycl_function_attributes)
30443044
<< AL << A->getSpelling();
@@ -3047,11 +3047,11 @@ static bool checkWorkGroupSizeValues(Sema &S, Decl *D, const ParsedAttr &AL,
30473047
}
30483048

30493049
if (const auto *A = D->getAttr<ReqdWorkGroupSizeAttr>()) {
3050-
if (!((getExprValue(AL.getArgAsExpr(2), S.getASTContext()) >=
3050+
if (!((getExprValue(AL.getArgAsExpr(0), S.getASTContext()) >=
30513051
getExprValue(A->getXDim(), S.getASTContext())) &&
30523052
(getExprValue(AL.getArgAsExpr(1), S.getASTContext()) >=
30533053
getExprValue(A->getYDim(), S.getASTContext())) &&
3054-
(getExprValue(AL.getArgAsExpr(0), S.getASTContext()) >=
3054+
(getExprValue(AL.getArgAsExpr(2), S.getASTContext()) >=
30553055
getExprValue(A->getZDim(), S.getASTContext())))) {
30563056
S.Diag(AL.getLoc(), diag::err_conflicting_sycl_function_attributes)
30573057
<< AL << A->getSpelling();
@@ -3172,10 +3172,6 @@ static void handleWorkGroupSize(Sema &S, Decl *D, const ParsedAttr &AL) {
31723172
return;
31733173
}
31743174

3175-
// For a SYCLDevice WorkGroupAttr arguments are reversed.
3176-
if (S.getLangOpts().SYCLIsDevice)
3177-
std::swap(XDimExpr, ZDimExpr);
3178-
31793175
if (WorkGroupAttr *ExistingAttr = D->getAttr<WorkGroupAttr>()) {
31803176
Optional<llvm::APSInt> XDimVal =
31813177
XDimExpr->getIntegerConstantExpr(S.getASTContext());

‎clang/test/CodeGenSYCL/intel-max-work-group-size.cpp

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,11 @@ class Foo {
1010
[[intel::max_work_group_size(1, 1, 1)]] void operator()() const {}
1111
};
1212

13+
class Bar {
14+
public:
15+
[[intel::max_work_group_size(1, 3, 6)]] void operator()() const {}
16+
};
17+
1318
template <int SIZE, int SIZE1, int SIZE2>
1419
class Functor {
1520
public:
@@ -27,10 +32,13 @@ int main() {
2732
h.single_task<class kernel_name2>(
2833
[]() [[intel::max_work_group_size(8, 8, 8)]]{});
2934

35+
Bar bar;
36+
h.single_task<class kernel_name3>(bar);
37+
3038
Functor<2, 2, 2> f;
31-
h.single_task<class kernel_name3>(f);
39+
h.single_task<class kernel_name4>(f);
3240

33-
h.single_task<class kernel_name4>([]() {
41+
h.single_task<class kernel_name5>([]() {
3442
func<4, 4, 4>();
3543
});
3644
});
@@ -39,9 +47,11 @@ int main() {
3947

4048
// CHECK: define spir_kernel void @{{.*}}kernel_name1"() #0 {{.*}} !max_work_group_size ![[NUM1:[0-9]+]]
4149
// CHECK: define spir_kernel void @{{.*}}kernel_name2"() #0 {{.*}} !max_work_group_size ![[NUM8:[0-9]+]]
42-
// CHECK: define spir_kernel void @{{.*}}kernel_name3"() #0 {{.*}} !max_work_group_size ![[NUM2:[0-9]+]]
43-
// CHECK: define spir_kernel void @{{.*}}kernel_name4"() #0 {{.*}} !max_work_group_size ![[NUM4:[0-9]+]]
50+
// CHECK: define spir_kernel void @{{.*}}kernel_name3"() #0 {{.*}} !max_work_group_size ![[NUM6:[0-9]+]]
51+
// CHECK: define spir_kernel void @{{.*}}kernel_name4"() #0 {{.*}} !max_work_group_size ![[NUM2:[0-9]+]]
52+
// CHECK: define spir_kernel void @{{.*}}kernel_name5"() #0 {{.*}} !max_work_group_size ![[NUM4:[0-9]+]]
4453
// CHECK: ![[NUM1]] = !{i32 1, i32 1, i32 1}
4554
// CHECK: ![[NUM8]] = !{i32 8, i32 8, i32 8}
55+
// CHECK: ![[NUM6]] = !{i32 6, i32 3, i32 1}
4656
// CHECK: ![[NUM2]] = !{i32 2, i32 2, i32 2}
4757
// CHECK: ![[NUM4]] = !{i32 4, i32 4, i32 4}

‎clang/test/CodeGenSYCL/reqd-work-group-size.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,9 @@ int main() {
4545
h.single_task<class kernel_name5>([]() {
4646
func<8, 4, 4>();
4747
});
48+
49+
h.single_task<class kernel_name6>(
50+
[]() [[cl::reqd_work_group_size(1, 8, 2)]]{});
4851
});
4952
return 0;
5053
}
@@ -54,8 +57,10 @@ int main() {
5457
// CHECK: define spir_kernel void @{{.*}}kernel_name3"() #0 {{.*}} !reqd_work_group_size ![[WGSIZE88:[0-9]+]]
5558
// CHECK: define spir_kernel void @{{.*}}kernel_name4"() #0 {{.*}} !reqd_work_group_size ![[WGSIZE22:[0-9]+]]
5659
// CHECK: define spir_kernel void @{{.*}}kernel_name5"() #0 {{.*}} !reqd_work_group_size ![[WGSIZE44:[0-9]+]]
60+
// CHECK: define spir_kernel void @{{.*}}kernel_name6"() #0 {{.*}} !reqd_work_group_size ![[WGSIZE2:[0-9]+]]
5761
// CHECK: ![[WGSIZE32]] = !{i32 16, i32 16, i32 32}
5862
// CHECK: ![[WGSIZE8]] = !{i32 1, i32 1, i32 8}
5963
// CHECK: ![[WGSIZE88]] = !{i32 8, i32 8, i32 8}
6064
// CHECK: ![[WGSIZE22]] = !{i32 2, i32 2, i32 2}
6165
// CHECK: ![[WGSIZE44]] = !{i32 4, i32 4, i32 8}
66+
// CHECK: ![[WGSIZE2]] = !{i32 2, i32 8, i32 1}

‎clang/test/SemaSYCL/intel-max-global-work-dim-device.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,13 +94,13 @@ int main() {
9494
h.single_task<class test_kernel5>(TRIFuncObjGood2());
9595
// CHECK-LABEL: FunctionDecl {{.*}}test_kernel5
9696
// CHECK: ReqdWorkGroupSizeAttr {{.*}}
97-
// // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
98-
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
9997
// CHECK-NEXT: IntegerLiteral{{.*}}4{{$}}
100-
// CHECK: SYCLIntelMaxWorkGroupSizeAttr {{.*}}
101-
// // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
10298
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
99+
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
100+
// CHECK: SYCLIntelMaxWorkGroupSizeAttr {{.*}}
103101
// CHECK-NEXT: IntegerLiteral{{.*}}8{{$}}
102+
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
103+
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
104104
// CHECK: SYCLIntelMaxGlobalWorkDimAttr {{.*}}
105105
// CHECK-NEXT: IntegerLiteral{{.*}}3{{$}}
106106
#ifdef TRIGGER_ERROR

‎clang/test/SemaSYCL/intel-max-work-group-size-device.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,11 +71,11 @@ int main() {
7171

7272
// CHECK-LABEL: FunctionDecl {{.*}}test_kernel4
7373
// CHECK: SYCLIntelMaxWorkGroupSizeAttr {{.*}}
74-
// CHECK-NEXT: UnaryOperator{{.*}} 'int' prefix '-'
7574
// CHECK-NEXT: IntegerLiteral{{.*}}8{{$}}
7675
// CHECK-NEXT: IntegerLiteral{{.*}}8{{$}}
76+
// CHECK-NEXT: UnaryOperator{{.*}} 'int' prefix '-'
7777
// CHECK-NEXT: IntegerLiteral{{.*}}8{{$}}
78-
// expected-warning@+2{{the resulting value of the 'max_work_group_size' attribute first parameter is always non-negative after implicit conversion}}
78+
// expected-warning@+2{{the resulting value of the 'max_work_group_size' attribute third parameter is always non-negative after implicit conversion}}
7979
h.single_task<class test_kernel4>(
8080
[]() [[intel::max_work_group_size(8, 8, -8)]]{});
8181

‎clang/test/SemaSYCL/intel-reqd-work-group-size-device.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -129,14 +129,14 @@ int main() {
129129

130130
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name1
131131
// CHECK: ReqdWorkGroupSizeAttr {{.*}}
132+
// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}}
132133
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
133134
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
134-
// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}}
135135
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name2
136136
// CHECK: ReqdWorkGroupSizeAttr {{.*}}
137+
// CHECK-NEXT: IntegerLiteral{{.*}}4{{$}}
137138
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
138139
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
139-
// CHECK-NEXT: IntegerLiteral{{.*}}4{{$}}
140140
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name3
141141
// CHECK: ReqdWorkGroupSizeAttr {{.*}}
142142
// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}}
@@ -149,10 +149,10 @@ int main() {
149149
// CHECK-NEXT: IntegerLiteral{{.*}}128{{$}}
150150
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name5
151151
// CHECK: ReqdWorkGroupSizeAttr {{.*}}
152-
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
152+
// CHECK-NEXT: IntegerLiteral{{.*}}32{{$}}
153153
// CHECK-NEXT: UnaryOperator{{.*}} 'int' prefix '-'
154154
// CHECK-NEXT: IntegerLiteral{{.*}}4{{$}}
155-
// CHECK-NEXT: IntegerLiteral{{.*}}32{{$}}
155+
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
156156
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name6
157157
// CHECK: ReqdWorkGroupSizeAttr {{.*}}
158158
// CHECK-NEXT: IntegerLiteral{{.*}}32{{$}}

‎clang/test/SemaSYCL/reqd-work-group-size-device.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,14 +106,14 @@ int main() {
106106

107107
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name1
108108
// CHECK: ReqdWorkGroupSizeAttr {{.*}}
109+
// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}}
109110
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
110111
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
111-
// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}}
112112
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name2
113113
// CHECK: ReqdWorkGroupSizeAttr {{.*}}
114+
// CHECK-NEXT: IntegerLiteral{{.*}}4{{$}}
114115
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
115116
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
116-
// CHECK-NEXT: IntegerLiteral{{.*}}4{{$}}
117117
// CHECK: FunctionDecl {{.*}} {{.*}}kernel_name3
118118
// CHECK: ReqdWorkGroupSizeAttr {{.*}}
119119
// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}}

‎clang/test/SemaSYCL/sycl-device-intel-reqd-work-group-size-template.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,13 +41,13 @@ int main() {
4141
// CHECK: ReqdWorkGroupSizeAttr {{.*}}
4242
// CHECK: SubstNonTypeTemplateParmExpr {{.*}}
4343
// CHECK-NEXT: NonTypeTemplateParmDecl {{.*}}
44-
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
44+
// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}}
4545
// CHECK: SubstNonTypeTemplateParmExpr {{.*}}
4646
// CHECK-NEXT: NonTypeTemplateParmDecl {{.*}}
4747
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
4848
// CHECK: SubstNonTypeTemplateParmExpr {{.*}}
4949
// CHECK-NEXT: NonTypeTemplateParmDecl {{.*}}
50-
// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}}
50+
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
5151

5252
// Test that checks template parameter suppport on function.
5353
template <int N, int N1, int N2>

‎clang/test/SemaSYCL/sycl-device-reqd-work-group-size-template.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,13 +41,13 @@ int main() {
4141
// CHECK: ReqdWorkGroupSizeAttr {{.*}}
4242
// CHECK: SubstNonTypeTemplateParmExpr {{.*}}
4343
// CHECK-NEXT: NonTypeTemplateParmDecl {{.*}}
44-
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
44+
// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}}
4545
// CHECK: SubstNonTypeTemplateParmExpr {{.*}}
4646
// CHECK-NEXT: NonTypeTemplateParmDecl {{.*}}
4747
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
4848
// CHECK: SubstNonTypeTemplateParmExpr {{.*}}
4949
// CHECK-NEXT: NonTypeTemplateParmDecl {{.*}}
50-
// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}}
50+
// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}}
5151

5252
// Test that checks template parameter suppport on function.
5353
template <int N, int N1, int N2>

0 commit comments

Comments
 (0)