From caa783e9c91d0f27a8e6ca1dccf14ecc05324606 Mon Sep 17 00:00:00 2001 From: Soumi Manna Date: Mon, 28 Dec 2020 09:13:04 -0800 Subject: [PATCH 1/2] [SYCL] Disable swapping dimension values of WorkGroupAttr arguments for semantic checks This is a followup comments on https://github.com/intel/llvm/pull/2906#discussion_r546940763 and https://github.com/intel/llvm/pull/2906#discussion_r548524994. 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 --- clang/lib/CodeGen/CodeGenFunction.cpp | 10 ++++++++++ clang/lib/Sema/SemaDeclAttr.cpp | 16 ++++++---------- .../CodeGenSYCL/intel-max-work-group-size.cpp | 18 ++++++++++++++---- .../test/CodeGenSYCL/reqd-work-group-size.cpp | 5 +++++ .../intel-max-global-work-dim-device.cpp | 8 ++++---- .../intel-max-work-group-size-device.cpp | 4 ++-- .../intel-reqd-work-group-size-device.cpp | 8 ++++---- .../SemaSYCL/reqd-work-group-size-device.cpp | 4 ++-- ...ice-intel-reqd-work-group-size-template.cpp | 4 ++-- ...cl-device-reqd-work-group-size-template.cpp | 4 ++-- 10 files changed, 51 insertions(+), 30 deletions(-) diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp index 0ad36d5d79168..75e9304bc3046 100644 --- a/clang/lib/CodeGen/CodeGenFunction.cpp +++ b/clang/lib/CodeGen/CodeGenFunction.cpp @@ -627,6 +627,11 @@ void CodeGenFunction::EmitOpenCLKernelMetadata(const FunctionDecl *FD, A->getYDim()->getIntegerConstantExpr(FD->getASTContext()); Optional ZDimVal = A->getZDim()->getIntegerConstantExpr(FD->getASTContext()); + + // For a SYCLDevice ReqdWorkGroupSizeAttr arguments are reversed. + if (getLangOpts().SYCLIsDevice) + std::swap(XDimVal, ZDimVal); + llvm::Metadata *AttrMDArgs[] = { llvm::ConstantAsMetadata::get( Builder.getInt32(XDimVal->getZExtValue())), @@ -702,6 +707,11 @@ void CodeGenFunction::EmitOpenCLKernelMetadata(const FunctionDecl *FD, A->getYDim()->getIntegerConstantExpr(FD->getASTContext()); Optional ZDimVal = A->getZDim()->getIntegerConstantExpr(FD->getASTContext()); + + // For a SYCLDevice SYCLIntelMaxWorkGroupSizeAttr arguments are reversed. + if (getLangOpts().SYCLIsDevice) + std::swap(XDimVal, ZDimVal); + llvm::Metadata *AttrMDArgs[] = { llvm::ConstantAsMetadata::get( Builder.getInt32(XDimVal->getZExtValue())), diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index 75decacd73c66..d86f5af065694 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/clang/lib/Sema/SemaDeclAttr.cpp @@ -3026,19 +3026,19 @@ static bool checkWorkGroupSizeValues(Sema &S, Decl *D, const ParsedAttr &AL, A->getValue()->getIntegerConstantExpr(S.Context)->getSExtValue(); if (AttrValue == 0) { Result &= - checkZeroDim(A, getExprValue(AL.getArgAsExpr(2), S.getASTContext()), + checkZeroDim(A, getExprValue(AL.getArgAsExpr(0), S.getASTContext()), getExprValue(AL.getArgAsExpr(1), S.getASTContext()), - getExprValue(AL.getArgAsExpr(0), S.getASTContext()), + getExprValue(AL.getArgAsExpr(2), S.getASTContext()), /*ReverseAttrs=*/true); } } if (const auto *A = D->getAttr()) { - if (!((getExprValue(AL.getArgAsExpr(2), S.getASTContext()) <= + if (!((getExprValue(AL.getArgAsExpr(0), S.getASTContext()) <= getExprValue(A->getXDim(), S.getASTContext())) && (getExprValue(AL.getArgAsExpr(1), S.getASTContext()) <= getExprValue(A->getYDim(), S.getASTContext())) && - (getExprValue(AL.getArgAsExpr(0), S.getASTContext()) <= + (getExprValue(AL.getArgAsExpr(2), S.getASTContext()) <= getExprValue(A->getZDim(), S.getASTContext())))) { S.Diag(AL.getLoc(), diag::err_conflicting_sycl_function_attributes) << AL << A->getSpelling(); @@ -3047,11 +3047,11 @@ static bool checkWorkGroupSizeValues(Sema &S, Decl *D, const ParsedAttr &AL, } if (const auto *A = D->getAttr()) { - if (!((getExprValue(AL.getArgAsExpr(2), S.getASTContext()) >= + if (!((getExprValue(AL.getArgAsExpr(0), S.getASTContext()) >= getExprValue(A->getXDim(), S.getASTContext())) && (getExprValue(AL.getArgAsExpr(1), S.getASTContext()) >= getExprValue(A->getYDim(), S.getASTContext())) && - (getExprValue(AL.getArgAsExpr(0), S.getASTContext()) >= + (getExprValue(AL.getArgAsExpr(2), S.getASTContext()) >= getExprValue(A->getZDim(), S.getASTContext())))) { S.Diag(AL.getLoc(), diag::err_conflicting_sycl_function_attributes) << AL << A->getSpelling(); @@ -3172,10 +3172,6 @@ static void handleWorkGroupSize(Sema &S, Decl *D, const ParsedAttr &AL) { return; } - // For a SYCLDevice WorkGroupAttr arguments are reversed. - if (S.getLangOpts().SYCLIsDevice) - std::swap(XDimExpr, ZDimExpr); - if (WorkGroupAttr *ExistingAttr = D->getAttr()) { Optional XDimVal = XDimExpr->getIntegerConstantExpr(S.getASTContext()); diff --git a/clang/test/CodeGenSYCL/intel-max-work-group-size.cpp b/clang/test/CodeGenSYCL/intel-max-work-group-size.cpp index 3d6a1a85b186c..575177da51106 100644 --- a/clang/test/CodeGenSYCL/intel-max-work-group-size.cpp +++ b/clang/test/CodeGenSYCL/intel-max-work-group-size.cpp @@ -10,6 +10,11 @@ class Foo { [[intel::max_work_group_size(1, 1, 1)]] void operator()() const {} }; +class Bar { +public: + [[intel::max_work_group_size(1, 3, 6)]] void operator()() const {} +}; + template class Functor { public: @@ -27,10 +32,13 @@ int main() { h.single_task( []() [[intel::max_work_group_size(8, 8, 8)]]{}); + Bar bar; + h.single_task(bar); + Functor<2, 2, 2> f; - h.single_task(f); + h.single_task(f); - h.single_task([]() { + h.single_task([]() { func<4, 4, 4>(); }); }); @@ -39,9 +47,11 @@ int main() { // CHECK: define spir_kernel void @{{.*}}kernel_name1"() #0 {{.*}} !max_work_group_size ![[NUM1:[0-9]+]] // CHECK: define spir_kernel void @{{.*}}kernel_name2"() #0 {{.*}} !max_work_group_size ![[NUM8:[0-9]+]] -// CHECK: define spir_kernel void @{{.*}}kernel_name3"() #0 {{.*}} !max_work_group_size ![[NUM2:[0-9]+]] -// CHECK: define spir_kernel void @{{.*}}kernel_name4"() #0 {{.*}} !max_work_group_size ![[NUM4:[0-9]+]] +// CHECK: define spir_kernel void @{{.*}}kernel_name3"() #0 {{.*}} !max_work_group_size ![[NUM6:[0-9]+]] +// CHECK: define spir_kernel void @{{.*}}kernel_name4"() #0 {{.*}} !max_work_group_size ![[NUM2:[0-9]+]] +// CHECK: define spir_kernel void @{{.*}}kernel_name5"() #0 {{.*}} !max_work_group_size ![[NUM4:[0-9]+]] // CHECK: ![[NUM1]] = !{i32 1, i32 1, i32 1} // CHECK: ![[NUM8]] = !{i32 8, i32 8, i32 8} +// CHECK: ![[NUM6]] = !{i32 6, i32 3, i32 1} // CHECK: ![[NUM2]] = !{i32 2, i32 2, i32 2} // CHECK: ![[NUM4]] = !{i32 4, i32 4, i32 4} diff --git a/clang/test/CodeGenSYCL/reqd-work-group-size.cpp b/clang/test/CodeGenSYCL/reqd-work-group-size.cpp index 5ef0ad78108dc..e0578750af603 100644 --- a/clang/test/CodeGenSYCL/reqd-work-group-size.cpp +++ b/clang/test/CodeGenSYCL/reqd-work-group-size.cpp @@ -45,6 +45,9 @@ int main() { h.single_task([]() { func<8, 4, 4>(); }); + + h.single_task( + []() [[cl::reqd_work_group_size(1, 8, 2)]]{}); }); return 0; } @@ -54,8 +57,10 @@ int main() { // CHECK: define spir_kernel void @{{.*}}kernel_name3"() #0 {{.*}} !reqd_work_group_size ![[WGSIZE88:[0-9]+]] // CHECK: define spir_kernel void @{{.*}}kernel_name4"() #0 {{.*}} !reqd_work_group_size ![[WGSIZE22:[0-9]+]] // CHECK: define spir_kernel void @{{.*}}kernel_name5"() #0 {{.*}} !reqd_work_group_size ![[WGSIZE44:[0-9]+]] +// CHECK: define spir_kernel void @{{.*}}kernel_name6"() #0 {{.*}} !reqd_work_group_size ![[WGSIZE2:[0-9]+]] // CHECK: ![[WGSIZE32]] = !{i32 16, i32 16, i32 32} // CHECK: ![[WGSIZE8]] = !{i32 1, i32 1, i32 8} // CHECK: ![[WGSIZE88]] = !{i32 8, i32 8, i32 8} // CHECK: ![[WGSIZE22]] = !{i32 2, i32 2, i32 2} // CHECK: ![[WGSIZE44]] = !{i32 4, i32 4, i32 8} +// CHECK: ![[WGSIZE2]] = !{i32 2, i32 8, i32 1} diff --git a/clang/test/SemaSYCL/intel-max-global-work-dim-device.cpp b/clang/test/SemaSYCL/intel-max-global-work-dim-device.cpp index cb3e359edd70b..7c462167c97ac 100644 --- a/clang/test/SemaSYCL/intel-max-global-work-dim-device.cpp +++ b/clang/test/SemaSYCL/intel-max-global-work-dim-device.cpp @@ -94,13 +94,13 @@ int main() { h.single_task(TRIFuncObjGood2()); // CHECK-LABEL: FunctionDecl {{.*}}test_kernel5 // CHECK: ReqdWorkGroupSizeAttr {{.*}} - // // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} + // // CHECK-NEXT: IntegerLiteral{{.*}}4{{$}} + // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} - // CHECK-NEXT: IntegerLiteral{{.*}}4{{$}} // CHECK: SYCLIntelMaxWorkGroupSizeAttr {{.*}} - // // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} + // // CHECK-NEXT: IntegerLiteral{{.*}}8{{$}} + // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} - // CHECK-NEXT: IntegerLiteral{{.*}}8{{$}} // CHECK: SYCLIntelMaxGlobalWorkDimAttr {{.*}} // CHECK-NEXT: IntegerLiteral{{.*}}3{{$}} #ifdef TRIGGER_ERROR diff --git a/clang/test/SemaSYCL/intel-max-work-group-size-device.cpp b/clang/test/SemaSYCL/intel-max-work-group-size-device.cpp index 779b85b10cdaa..509321953309a 100644 --- a/clang/test/SemaSYCL/intel-max-work-group-size-device.cpp +++ b/clang/test/SemaSYCL/intel-max-work-group-size-device.cpp @@ -71,11 +71,11 @@ int main() { // CHECK-LABEL: FunctionDecl {{.*}}test_kernel4 // CHECK: SYCLIntelMaxWorkGroupSizeAttr {{.*}} - // CHECK-NEXT: UnaryOperator{{.*}} 'int' prefix '-' // CHECK-NEXT: IntegerLiteral{{.*}}8{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}8{{$}} + // CHECK-NEXT: UnaryOperator{{.*}} 'int' prefix '-' // CHECK-NEXT: IntegerLiteral{{.*}}8{{$}} - // expected-warning@+2{{the resulting value of the 'max_work_group_size' attribute first parameter is always non-negative after implicit conversion}} + // expected-warning@+2{{the resulting value of the 'max_work_group_size' attribute third parameter is always non-negative after implicit conversion}} h.single_task( []() [[intel::max_work_group_size(8, 8, -8)]]{}); diff --git a/clang/test/SemaSYCL/intel-reqd-work-group-size-device.cpp b/clang/test/SemaSYCL/intel-reqd-work-group-size-device.cpp index 5e7e9fe900944..d47546e358c73 100644 --- a/clang/test/SemaSYCL/intel-reqd-work-group-size-device.cpp +++ b/clang/test/SemaSYCL/intel-reqd-work-group-size-device.cpp @@ -129,14 +129,14 @@ int main() { // CHECK: FunctionDecl {{.*}} {{.*}}kernel_name1 // CHECK: ReqdWorkGroupSizeAttr {{.*}} +// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} -// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}} // CHECK: FunctionDecl {{.*}} {{.*}}kernel_name2 // CHECK: ReqdWorkGroupSizeAttr {{.*}} +// CHECK-NEXT: IntegerLiteral{{.*}}4{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} -// CHECK-NEXT: IntegerLiteral{{.*}}4{{$}} // CHECK: FunctionDecl {{.*}} {{.*}}kernel_name3 // CHECK: ReqdWorkGroupSizeAttr {{.*}} // CHECK-NEXT: IntegerLiteral{{.*}}16{{$}} @@ -149,10 +149,10 @@ int main() { // CHECK-NEXT: IntegerLiteral{{.*}}128{{$}} // CHECK: FunctionDecl {{.*}} {{.*}}kernel_name5 // CHECK: ReqdWorkGroupSizeAttr {{.*}} -// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} +// CHECK-NEXT: IntegerLiteral{{.*}}32{{$}} // CHECK-NEXT: UnaryOperator{{.*}} 'int' prefix '-' // CHECK-NEXT: IntegerLiteral{{.*}}4{{$}} -// CHECK-NEXT: IntegerLiteral{{.*}}32{{$}} +// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // CHECK: FunctionDecl {{.*}} {{.*}}kernel_name6 // CHECK: ReqdWorkGroupSizeAttr {{.*}} // CHECK-NEXT: IntegerLiteral{{.*}}32{{$}} diff --git a/clang/test/SemaSYCL/reqd-work-group-size-device.cpp b/clang/test/SemaSYCL/reqd-work-group-size-device.cpp index 441516a57ded9..dff92cce4e6ef 100644 --- a/clang/test/SemaSYCL/reqd-work-group-size-device.cpp +++ b/clang/test/SemaSYCL/reqd-work-group-size-device.cpp @@ -106,14 +106,14 @@ int main() { // CHECK: FunctionDecl {{.*}} {{.*}}kernel_name1 // CHECK: ReqdWorkGroupSizeAttr {{.*}} +// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} -// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}} // CHECK: FunctionDecl {{.*}} {{.*}}kernel_name2 // CHECK: ReqdWorkGroupSizeAttr {{.*}} +// CHECK-NEXT: IntegerLiteral{{.*}}4{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} -// CHECK-NEXT: IntegerLiteral{{.*}}4{{$}} // CHECK: FunctionDecl {{.*}} {{.*}}kernel_name3 // CHECK: ReqdWorkGroupSizeAttr {{.*}} // CHECK-NEXT: IntegerLiteral{{.*}}16{{$}} diff --git a/clang/test/SemaSYCL/sycl-device-intel-reqd-work-group-size-template.cpp b/clang/test/SemaSYCL/sycl-device-intel-reqd-work-group-size-template.cpp index 0e00286f9a287..c297f4e15e091 100644 --- a/clang/test/SemaSYCL/sycl-device-intel-reqd-work-group-size-template.cpp +++ b/clang/test/SemaSYCL/sycl-device-intel-reqd-work-group-size-template.cpp @@ -41,13 +41,13 @@ int main() { // CHECK: ReqdWorkGroupSizeAttr {{.*}} // CHECK: SubstNonTypeTemplateParmExpr {{.*}} // CHECK-NEXT: NonTypeTemplateParmDecl {{.*}} -// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} +// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}} // CHECK: SubstNonTypeTemplateParmExpr {{.*}} // CHECK-NEXT: NonTypeTemplateParmDecl {{.*}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // CHECK: SubstNonTypeTemplateParmExpr {{.*}} // CHECK-NEXT: NonTypeTemplateParmDecl {{.*}} -// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}} +// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // Test that checks template parameter suppport on function. template diff --git a/clang/test/SemaSYCL/sycl-device-reqd-work-group-size-template.cpp b/clang/test/SemaSYCL/sycl-device-reqd-work-group-size-template.cpp index f46e40933b59f..b981f524b2b0b 100644 --- a/clang/test/SemaSYCL/sycl-device-reqd-work-group-size-template.cpp +++ b/clang/test/SemaSYCL/sycl-device-reqd-work-group-size-template.cpp @@ -41,13 +41,13 @@ int main() { // CHECK: ReqdWorkGroupSizeAttr {{.*}} // CHECK: SubstNonTypeTemplateParmExpr {{.*}} // CHECK-NEXT: NonTypeTemplateParmDecl {{.*}} -// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} +// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}} // CHECK: SubstNonTypeTemplateParmExpr {{.*}} // CHECK-NEXT: NonTypeTemplateParmDecl {{.*}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // CHECK: SubstNonTypeTemplateParmExpr {{.*}} // CHECK-NEXT: NonTypeTemplateParmDecl {{.*}} -// CHECK-NEXT: IntegerLiteral{{.*}}16{{$}} +// CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // Test that checks template parameter suppport on function. template From d4581c33f8afac2fbb1e6c2f8fbab52ed2c21000 Mon Sep 17 00:00:00 2001 From: Soumi Manna Date: Tue, 29 Dec 2020 06:25:27 -0800 Subject: [PATCH 2/2] remove duplicate comments from tests Signed-off-by: Soumi Manna --- clang/test/SemaSYCL/intel-max-global-work-dim-device.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/test/SemaSYCL/intel-max-global-work-dim-device.cpp b/clang/test/SemaSYCL/intel-max-global-work-dim-device.cpp index 7c462167c97ac..251f7257aa943 100644 --- a/clang/test/SemaSYCL/intel-max-global-work-dim-device.cpp +++ b/clang/test/SemaSYCL/intel-max-global-work-dim-device.cpp @@ -94,11 +94,11 @@ int main() { h.single_task(TRIFuncObjGood2()); // CHECK-LABEL: FunctionDecl {{.*}}test_kernel5 // CHECK: ReqdWorkGroupSizeAttr {{.*}} - // // CHECK-NEXT: IntegerLiteral{{.*}}4{{$}} + // CHECK-NEXT: IntegerLiteral{{.*}}4{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // CHECK: SYCLIntelMaxWorkGroupSizeAttr {{.*}} - // // CHECK-NEXT: IntegerLiteral{{.*}}8{{$}} + // CHECK-NEXT: IntegerLiteral{{.*}}8{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // CHECK-NEXT: IntegerLiteral{{.*}}1{{$}} // CHECK: SYCLIntelMaxGlobalWorkDimAttr {{.*}}