Skip to content

Commit d98b6dc

Browse files
smanna12smanna12
committed
Address @aaron's review comments
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
1 parent c1ba909 commit d98b6dc

File tree

3 files changed

+28
-60
lines changed

3 files changed

+28
-60
lines changed

‎clang/lib/Sema/SemaDeclAttr.cpp

Lines changed: 21 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3451,27 +3451,28 @@ static void handleWorkGroupSizeHint(Sema &S, Decl *D, const ParsedAttr &AL) {
34513451
}
34523452

34533453
// Handles max_work_group_size attribute.
3454-
// If the declaration has a SYCLIntelMaxWorkGroupSizeAttr,
3455-
// check to see if the attribute holds equal values (1, 1, 1).
3456-
// Returns true if the attribute values are not equal to one.
3457-
static bool InvalidWorkGroupSizeAttrs(const Expr *E, const Expr *E1,
3458-
const Expr *E2, const Expr *E3) {
3454+
// If the [[intel::max_work_group_size(X, Y, Z)]] attribute is specified on a
3455+
// declaration along with [[intel::max_global_work_dim()]] attribute,
3456+
// check to see if all arguments of [[intel::max_work_group_size(X, Y, Z)]]
3457+
// attribute hold value 1 in case the argument of
3458+
// [[intel::max_global_work_dim()]] attribute equals to 0.
3459+
static bool InvalidWorkGroupSizeAttrs(const Expr *MGValue, const Expr *XDim,
3460+
const Expr *YDim, const Expr *ZDim) {
34593461
// If any of the operand is still value dependent, we can't test anything.
3460-
const auto *CE = dyn_cast<ConstantExpr>(E);
3461-
const auto *CE1 = dyn_cast<ConstantExpr>(E1);
3462-
const auto *CE2 = dyn_cast<ConstantExpr>(E2);
3463-
const auto *CE3 = dyn_cast<ConstantExpr>(E3);
3462+
const auto *MGValueExpr = dyn_cast<ConstantExpr>(MGValue);
3463+
const auto *XDimExpr = dyn_cast<ConstantExpr>(XDim);
3464+
const auto *YDimExpr = dyn_cast<ConstantExpr>(YDim);
3465+
const auto *ZDimExpr = dyn_cast<ConstantExpr>(ZDim);
34643466

3465-
if (!CE || !CE1 || !CE2 || !CE3)
3467+
if (!MGValueExpr || !XDimExpr || !YDimExpr || !ZDimExpr)
34663468
return false;
34673469

34683470
// Otherwise, check if the attribute values are equal to one.
3469-
if (CE->getResultAsAPSInt() == 0 &&
3470-
(CE1->getResultAsAPSInt() != 1 || CE2->getResultAsAPSInt() != 1 ||
3471-
CE3->getResultAsAPSInt() != 1)) {
3472-
return true;
3473-
}
3474-
return false;
3471+
return (MGValueExpr->getResultAsAPSInt() == 0 &&
3472+
(XDimExpr->getResultAsAPSInt() != 1 ||
3473+
YDimExpr->getResultAsAPSInt() != 1 ||
3474+
ZDimExpr->getResultAsAPSInt() != 1));
3475+
34753476
}
34763477

34773478
void Sema::AddSYCLIntelMaxWorkGroupSizeAttr(Decl *D,
@@ -3489,17 +3490,10 @@ void Sema::AddSYCLIntelMaxWorkGroupSizeAttr(Decl *D,
34893490
return nullptr;
34903491
E = Res.get();
34913492

3492-
if (ArgVal.isNegative()) {
3493-
Diag(E->getExprLoc(),
3494-
diag::warn_attribute_requires_non_negative_integer_argument)
3495-
<< E->getType() << Context.UnsignedLongLongTy
3496-
<< E->getSourceRange();
3497-
return E;
3498-
}
3499-
3500-
if (ArgVal == 0) {
3501-
Diag(E->getExprLoc(), diag::err_attribute_argument_is_zero)
3502-
<< CI << E->getSourceRange();
3493+
// This attribute requires a strictly positive value.
3494+
if (ArgVal <= 0) {
3495+
Diag(E->getExprLoc(), diag::err_attribute_requires_positive_integer)
3496+
<< CI << /*positive*/ 0;
35033497
return nullptr;
35043498
}
35053499
}

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

Lines changed: 5 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -86,43 +86,17 @@ int main() {
8686
h.single_task<class test_kernel3>(
8787
[]() { func_do_not_ignore(); });
8888

89-
// CHECK-LABEL: FunctionDecl {{.*}}test_kernel4
90-
// CHECK: SYCLIntelMaxWorkGroupSizeAttr {{.*}}
91-
// CHECK-NEXT: ConstantExpr{{.*}}'int'
92-
// CHECK-NEXT: value: Int 8
93-
// CHECK-NEXT: IntegerLiteral{{.*}}8{{$}}
94-
// CHECK-NEXT: ConstantExpr{{.*}}'int'
95-
// CHECK-NEXT: value: Int 8
96-
// CHECK-NEXT: IntegerLiteral{{.*}}8{{$}}
97-
// CHECK-NEXT: ConstantExpr{{.*}}'int'
98-
// CHECK-NEXT: value: Int -8
99-
// CHECK-NEXT: UnaryOperator{{.*}} 'int' prefix '-'
100-
// CHECK-NEXT: IntegerLiteral{{.*}}8{{$}}
101-
// expected-warning@+2{{implicit conversion changes signedness: 'int' to 'unsigned long long'}}
89+
#ifdef TRIGGER_ERROR
10290
h.single_task<class test_kernel4>(
103-
[]() [[intel::max_work_group_size(8, 8, -8)]]{});
91+
[]() [[intel::max_work_group_size(8, 8, -8)]]{}); // expected-error{{'max_work_group_size' attribute requires a positive integral compile time constant expression}}
10492

105-
// CHECK-LABEL: FunctionDecl {{.*}}test_kernel5
106-
// CHECK: SYCLIntelMaxWorkGroupSizeAttr {{.*}}
107-
// CHECK-NEXT: ConstantExpr{{.*}}'int'
108-
// CHECK-NEXT: value: Int -8
109-
// CHECK-NEXT: UnaryOperator{{.*}} 'int' prefix '-'
110-
// CHECK-NEXT: IntegerLiteral{{.*}}8{{$}}
111-
// CHECK-NEXT: ConstantExpr{{.*}}'int'
112-
// CHECK-NEXT: value: Int 8
113-
// CHECK-NEXT: IntegerLiteral{{.*}}8{{$}}
114-
// CHECK-NEXT: ConstantExpr{{.*}}'int'
115-
// CHECK-NEXT: value: Int -8
116-
// CHECK-NEXT: UnaryOperator{{.*}} 'int' prefix '-'
117-
// CHECK-NEXT: IntegerLiteral{{.*}}8{{$}}
118-
// expected-warning@+2 2{{implicit conversion changes signedness: 'int' to 'unsigned long long'}}
11993
h.single_task<class test_kernel5>(
120-
[]() [[intel::max_work_group_size(-8, 8, -8)]]{});
121-
#ifdef TRIGGER_ERROR
94+
[]() [[intel::max_work_group_size(-8, 8, -8)]]{}); // expected-error 2{{'max_work_group_size' attribute requires a positive integral compile time constant expression}}
95+
12296
[[intel::max_work_group_size(1, 1, 1)]] int Var = 0; // expected-error{{'max_work_group_size' attribute only applies to functions}}
12397

12498
h.single_task<class test_kernel6>(
125-
[]() [[intel::max_work_group_size(0, 1, 3)]]{}); // expected-error{{'max_work_group_size' attribute must be greater than 0}}
99+
[]() [[intel::max_work_group_size(0, 1, 3)]]{}); // expected-error{{'max_work_group_size' attribute requires a positive integral compile time constant expression}}
126100

127101
h.single_task<class test_kernel7>(
128102
[]() [[intel::max_work_group_size(1.2f, 1, 3)]]{}); // expected-error{{integral constant expression must have integral or unscoped enumeration type, not 'float'}}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,9 @@ template <int X, int Y, int Z>
5050

5151
void instantiate() {
5252
f6<1>(); // OK
53-
// expected-warning@#f6 {{implicit conversion changes signedness: 'int' to 'unsigned long long'}}
53+
// expected-error@#f6 {{'max_work_group_size' attribute requires a positive integral compile time constant expression}}
5454
f6<-1>(); // expected-note {{in instantiation}}
55-
// expected-error@#f6 {{'max_work_group_size' attribute must be greater than 0}}
55+
// expected-error@#f6 {{'max_work_group_size' attribute requires a positive integral compile time constant expression}}
5656
f6<0>(); // expected-note {{in instantiation}}
5757
f7<1, 1, 1>(); // OK, args are the same on the redecl.
5858
// expected-warning@#f7 {{attribute 'max_work_group_size' is already applied with different arguments}}

0 commit comments

Comments
 (0)