[SYCL] remove SYCL 2020 incompatible tests #14261

AndreiZibrov · 2024-06-21T23:31:50Z

This PR removes the SYCL 2017 deprecated tests as a continuation of #14239 in order of #13411 and 13556

The list of faulty tests mentioned in description of #14239:
Clang :: Driver/clang-offload-bundler-bc-archive-support-linux.cpp
Clang :: Driver/clang-offload-wrapper-input-bc.cpp
Clang :: Driver/clang-offload-wrapper-input-multi-bc.cpp
Clang :: Driver/sycl-linker-wrapper-image.cpp
Clang :: SemaSYCL/lb_sm_90.cpp
have to be investigated separately due to no connection with SYCL 2017/1.2.1.

… device-function attribute propagation

AlexeySachkov · 2024-06-22T08:25:23Z

clang/test/Frontend/sycl-unnamed-lambdas-default.cpp

@@ -1,18 +1,14 @@
 // RUN: %clang_cc1 -fsycl-is-device %s -verify -DUNNAMED_LAMBDAS
 // RUN: %clang_cc1 -fsycl-is-host %s -verify -DUNNAMED_LAMBDAS
 //
-// RUN: %clang_cc1 -fsycl-is-host -sycl-std=2017 %s -verify
+// RUN: %clang_cc1 -fsycl-is-host -sycl-std=2020 %s -verify


I think that we can remove this RUN line, because it doesn't really check anything.

AlexeySachkov · 2024-06-22T08:26:38Z

clang/test/Frontend/sycl-unnamed-lambdas-default.cpp

-#if !defined(UNNAMED_LAMBDAS) && defined(__SYCL_UNNAMED_LAMBDA__)
-#error "Unnamed lambdas should NOT be enabled for SYCL2017, or when explicitly disabled"


This #if should be preserved, but we may want to tweak the message here. For RUN lines 7 and 8 we want unnamed lambas to be disabled and without this #if there will be no check if that actually happens.

AlexeySachkov · 2024-06-22T08:28:44Z

clang/test/Preprocessor/sycl-macro.cpp

-// RUN: %clang_cc1 %s -fsycl-id-queries-fit-in-int -fsycl-is-host -sycl-std=2017 -E -dM | FileCheck --check-prefix=CHECK-SYCL-STD %s
-// RUN: %clang_cc1 %s -fsycl-id-queries-fit-in-int -fsycl-is-device -sycl-std=2017 -E -dM | FileCheck --check-prefix=CHECK-SYCL-STD %s


We already have RUN lines for SYCL-2020, I don't think that we need more of them. Lets just drop these two and update CHECK-SYCL-STD-2020 check lines to include __SYCL_ID_QUERIES_FIT_IN_INT__

AlexeySachkov · 2024-06-22T08:29:24Z

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

@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -sycl-std=2017 -ast-dump %s | FileCheck %s
+// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -sycl-std=2020 -ast-dump %s | FileCheck %s

 // Test for AST of reqd_work_group_size kernel attribute in SYCL 1.2.1.


The comment should be updated

AlexeySachkov · 2024-06-22T08:33:13Z

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

-// Test of redeclaration of [[intel::max_work_group_size()]] and [[sycl::reqd_work_group_size()]].
-[[intel::no_global_work_offset]] void func1();
-[[intel::max_work_group_size(4, 4, 4)]] void func1();
-[[sycl::reqd_work_group_size(2, 2, 2)]] void func1() {}


It seems like those lines are used to check that we can apply more attributes to redeclarations and even though even if func1 is now unused, I would still preserve those lines.

If func1 is unused the redeclarations won't be checked unless you apply the attributes directly to the lambda or operator.

AlexeySachkov · 2024-06-22T08:34:38Z

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

-    // CHECK: FunctionDecl {{.*}} {{.*}}kernel_name5
-    // CHECK: SYCLReqdWorkGroupSizeAttr
-    // CHECK-NEXT:  ConstantExpr{{.*}}'int'
-    // CHECK-NEXT:  value: Int 32
-    // CHECK-NEXT:  IntegerLiteral{{.*}}32{{$}}
-    // CHECK-NEXT:  ConstantExpr{{.*}}'int'
-    // CHECK-NEXT:  value: Int 32
-    // CHECK-NEXT:  IntegerLiteral{{.*}}32{{$}}
-    // CHECK-NEXT:  ConstantExpr{{.*}}'int'
-    // CHECK-NEXT:  value: Int 32
-    // CHECK-NEXT:  IntegerLiteral{{.*}}32{{$}}
-    h.single_task<class kernel_name5>([]() [[sycl::reqd_work_group_size(32, 32, 32)]] {
-      f32x32x32();
-    });


It seems like without this piece of code we won't have a test where we check for AST based on the attribute that directly applies to a lambda. I would suggest dropping f32x32x32 call from the lambda, but keeping the "test case" itself.

AlexeySachkov · 2024-06-22T08:36:27Z

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

@@ -1,67 +0,0 @@
-// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -sycl-std=2017 -Wno-sycl-2017-compat -fsyntax-only -verify %s


Note to reviewers and myself: we have a positive test that checks that the attribute can be applied to lambdas and functors without any diagnostics in clang/test/SemaSYCL/reqd-sub-group-size.cpp. This test seems to be focused on propagation of the attribute from device functions to kernels which is not a thing in SYCL 2020 and therefore we don't need this test anymore.

bader · 2024-06-23T19:35:05Z

clang/test/CodeGenSYCL/intel-fpga-no-global-work-offset.cpp

@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -triple spir64-unknown-unknown -disable-llvm-passes -sycl-std=2017 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -triple spir64-unknown-unknown -disable-llvm-passes -sycl-std=2020 -emit-llvm -o - %s | FileCheck %s


-sycl-std=2020 is the default and the only supported version, so there is reason to set it explicitly. Right?

-sycl-std=2020 is the default and the only supported version, so there is reason to set it explicitly. Right?

Then I'd like us to agree:

all tests have to use the default version (except some specific)
or vise-versa: all tests have to use the explicit version (except some specific)

Otherwise, what will happen with all these clang/tests changed as you proposed in case of the next standard will be defaulted after 2020 version (eg 2024/2025/etc)?

Then I'd like us to agree:
all tests have to use the default version (except some specific)

As far as I can tell, this is the way tests written today. This patch introduces exceptions to this rule and that's why I raised the question.

this is the way tests written today
and that's why I raised the question.

Thank you for correcting my point of view, I see it now

find -path "*/test/*.cpp" -type f|xargs -I{} grep -anH '// RUN: %clang_cc1' {}|grep "\-sycl\-std="|awk '{print $1}'|wc -l 139 find -path "*/test/*.cpp" -type f|xargs -I{} grep -anH '// RUN: %clang_cc1' {}|grep -v "\-sycl\-std="|awk '{print $1}'|wc -l 23838

So as per your proposal the 's/(// RUN:.) -sycl-std=\d+ (.)/\1 \2/' is O.K.
But do you think that '// RUN.*(CHECK-SYCL(-STD-)?|-Wno-sycl-|-verify=sycl-)2020' should be handled as well?
There are 9 cases which I'm not sure how to resolve yet.

I'd move discussed changes to separate PR to not overload/mix this one. Are you O.K. with it?

So as per your proposal the 's/(// RUN:.) -sycl-std=\d+ (.)/\1 \2/' is O.K.

It's not okay. As you noted there are exceptions i.e. tests which validate -sycl-std=2020 option.

My request is to go over the tests where you changed -sycl-std=2017 to -sycl-std=2020 and consider dropping the option altogether.

Regarding separate PR. I don't see value in making these changes in separate commits. I prefer making the final change in this PR.

elizabethandrews

@smanna12 can you please take a look since you wrote most of these tests? I was hoping you can confirm we aren't losing any coverage

elizabethandrews · 2024-06-24T15:17:21Z

clang/test/CodeGenSYCL/intel-max-global-work-dim.cpp

@@ -29,17 +26,12 @@ int main() {

    Functor<2> f;
    h.single_task<class kernel_name3>(f);
-


I can't recall. Have we already removed attribute propagation logic from the compiler i.e. SemaSYCL.cpp. If not, we should.

I can't recall. Have we already removed attribute propagation logic from the compiler i.e. SemaSYCL.cpp. If not, we should.

afaik func() is a device function

So in case std-sycl-2020/default and stack of symbols func()<-kernel_name4 exist and checks NUM2 and kernel_name4 are used the line

./build/bin/llvm-lit -a -v clang/test/CodeGenSYCL/intel-max-global-work-dim.cpp
is producing this

build/bin/clang -cc1 -internal-isystem build/lib/clang/19/include -nostdsysteminc -fsycl-is-device -internal-isystem clang/test/CodeGenSYCL/Inputs -triple spir64-unknown-unknown -disable-llvm-passes -emit-llvm -o - clang/test/CodeGenSYCL/intel-max-global-work-dim.cpp | build/bin/FileCheck clang/test/CodeGenSYCL/intel-max-global-work-dim.cpp + build/bin/FileCheck clang/test/CodeGenSYCL/intel-max-global-work-dim.cpp + build/bin/clang -cc1 -internal-isystem build/lib/clang/19/include -nostdsysteminc -fsycl-is-device -internal-isystem clang/test/CodeGenSYCL/Inputs -triple spir64-unknown-unknown -disable-llvm-passes -emit-llvm -o - clang/test/CodeGenSYCL/intel-max-global-work-dim.cpp clang/test/CodeGenSYCL/intel-max-global-work-dim.cpp:43:11: error: CHECK: expected string not found in input

clang/test/CodeGenSYCL/intel-max-global-work-dim.cpp:43:11: error: CHECK: expected string not found in input // CHECK: define {{.*}}spir_kernel void @{{.*}}kernel_name4() #0 {{.*}} !max_global_work_dim ![[NUM2]]

https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#sec:device.attributes

"The C++ attributes that decorate kernels are now better described, and their position has changed so that they are applied directly to the kernel function. (Previously, they were applied to a device function that the kernel calls, and the implementation needed to propagate the information up to the enclosing kernel.) The old C++ attribute form is no longer included in the SYCL specification."

I understand. I was saying we have code doing this attribute propagation in SemaSYCL.cpp. That needs to be removed as well as part of this cleanup.

elizabethandrews · 2024-06-24T15:27:17Z

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

-// Test of redeclaration of [[intel::max_work_group_size()]] and [[sycl::reqd_work_group_size()]].
-[[intel::no_global_work_offset]] void func1();
-[[intel::max_work_group_size(4, 4, 4)]] void func1();
-[[sycl::reqd_work_group_size(2, 2, 2)]] void func1() {}


If func1 is unused the redeclarations won't be checked unless you apply the attributes directly to the lambda or operator.

elizabethandrews · 2024-06-24T16:16:51Z

clang/test/SemaSYCL/sycl-esimd-ast.cpp

@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -sycl-std=2017 -ast-dump %s | FileCheck %s
+// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -sycl-std=2020 -ast-dump %s | FileCheck %s

 // Tests for AST of sycl_explicit_simd function attribute in SYCL 1.2.1.


Comment needs to be updated

mdtoguchi

small driver test change LGTM

AndreiZibrov added 3 commits June 21, 2024 15:50

[SYCL] rest of -Wno-sycl-2017-compat removal in SemaSYCL tests

c54f698

[SYCL] removing -sycl-std=2017 related tests cases or updating to 2020

cdd55bc

[SYCL] getting rid of test cases based on -sycl-std=2020 incompatible…

4aba0cf

… device-function attribute propagation

AndreiZibrov requested review from a team as code owners June 21, 2024 23:31

AlexeySachkov reviewed Jun 22, 2024

View reviewed changes

bader reviewed Jun 23, 2024

View reviewed changes

elizabethandrews reviewed Jun 24, 2024

View reviewed changes

mdtoguchi approved these changes Jun 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] remove SYCL 2020 incompatible tests #14261

[SYCL] remove SYCL 2020 incompatible tests #14261

AndreiZibrov commented Jun 21, 2024

AlexeySachkov Jun 22, 2024

AlexeySachkov Jun 22, 2024

AlexeySachkov Jun 22, 2024

AlexeySachkov Jun 22, 2024

AlexeySachkov Jun 22, 2024

elizabethandrews Jun 24, 2024

AlexeySachkov Jun 22, 2024

AlexeySachkov Jun 22, 2024

bader Jun 23, 2024

AndreiZibrov Jun 24, 2024 •

edited

Loading

bader Jun 24, 2024

AndreiZibrov Jun 24, 2024

bader Jun 24, 2024

elizabethandrews left a comment

elizabethandrews Jun 24, 2024

AndreiZibrov Jun 24, 2024

elizabethandrews Jun 24, 2024

elizabethandrews Jun 24, 2024

elizabethandrews Jun 24, 2024

mdtoguchi left a comment

		#if !defined(UNNAMED_LAMBDAS) && defined(__SYCL_UNNAMED_LAMBDA__)
		#error "Unnamed lambdas should NOT be enabled for SYCL2017, or when explicitly disabled"

		// RUN: %clang_cc1 %s -fsycl-id-queries-fit-in-int -fsycl-is-host -sycl-std=2017 -E -dM \| FileCheck --check-prefix=CHECK-SYCL-STD %s
		// RUN: %clang_cc1 %s -fsycl-id-queries-fit-in-int -fsycl-is-device -sycl-std=2017 -E -dM \| FileCheck --check-prefix=CHECK-SYCL-STD %s

		@@ -1,67 +0,0 @@
		// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -sycl-std=2017 -Wno-sycl-2017-compat -fsyntax-only -verify %s

		@@ -1,4 +1,4 @@
		// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -triple spir64-unknown-unknown -disable-llvm-passes -sycl-std=2017 -emit-llvm -o - %s \| FileCheck %s
		// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -triple spir64-unknown-unknown -disable-llvm-passes -sycl-std=2020 -emit-llvm -o - %s \| FileCheck %s

		@@ -29,17 +26,12 @@ int main() {

		Functor<2> f;
		h.single_task<class kernel_name3>(f);

[SYCL] remove SYCL 2020 incompatible tests #14261

Are you sure you want to change the base?

[SYCL] remove SYCL 2020 incompatible tests #14261

Conversation

AndreiZibrov commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreiZibrov Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elizabethandrews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdtoguchi left a comment

Choose a reason for hiding this comment

AndreiZibrov Jun 24, 2024 •

edited

Loading