[NFC][SYCL] Update tests to reflect new offload model #14539

mdtoguchi · 2024-07-11T16:42:46Z

Update existing SYCL based driver tests to use the new offload model and retain the equivalent old offloading model tests be creating copies. These tests use the --offload-new-driver and --no-offload-new-driver options to force a particular model to follow.

srividya-sundaram · 2024-07-11T19:53:36Z

clang/test/Driver/clang-offload-bundler-bc-archive-support-linux.cpp

+
+////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+// Make three distinct BC files
+// RUN: %clangxx -fsycl -fsycl-device-only -DTYPE1 %s -o %t1.bc


Is --no-offload-new-driver not required here?
I see it added here for generating BC files.

It is required - will add.

srividya-sundaram · 2024-07-11T19:59:44Z

clang/test/Driver/ftarget-compile-fast-old-model.cpp

@@ -0,0 +1,20 @@
+// Test -ftarget-compile-fast behaviors


Some of the comments are updated with // Old offloading model only
Can be made consistent by updating all comments as necessary.

The files that are commented with 'Old offload model only' are tests that already exist that were not converted for the new model as those behaviors being tested do not make sense for the new model.

srividya-sundaram · 2024-07-11T20:00:24Z

clang/test/Driver/ftarget-compile-fast.cpp

@@ -1,20 +1,21 @@
 // Test -ftarget-compile-fast behaviors


Comment can be updated to mention the use of new offload driver

The files where an 'old-model' variant was added I do not update the comments to reflect. The naming should take care of that. Once the old model goes away, all of these tests will also be updated to remove the --offload-new-driver options being set.

srividya-sundaram · 2024-07-11T20:00:50Z

clang/test/Driver/sycl-add-default-spec-consts-image.cpp

-// RUN: %clang -### -fsycl -fsycl-add-default-spec-consts-image -fsycl-targets=spir64 2>&1 %s | FileCheck %s -check-prefix=CHECK-NON-AOT
+// RUN: %clang -### -fsycl --no-offload-new-driver -fsycl-add-default-spec-consts-image 2>&1 %s | FileCheck %s -check-prefix=CHECK-NON-AOT
+// RUN: %clang_cl -### -fsycl --no-offload-new-driver -fsycl-add-default-spec-consts-image 2>&1 %s | FileCheck %s -check-prefix=CHECK-NON-AOT
+// RUN: %clang -### -fsycl --no-offload-new-driver -fsycl-add-default-spec-consts-image -fsycl-targets=spir64 2>&1 %s | FileCheck %s -check-prefix=CHECK-NON-AOT
 // CHECK-NON-AOT: warning: -fsycl-add-default-spec-consts-image flag has an effect only in Ahead of Time Compilation mode (AOT)

 // Check that non-AOT target doesn't add command line option into sycl-post-link invocation
 // CHECK-NON-AOT-NOT: {{.*}}sycl-post-link{{.*}} "-generate-device-image-default-spec-consts"

 // Check that no warnings are issued in correct cases and "-generate-device-image-default-spec-consts" is passed to sycl-post-link


Comment update.

Can you clarify?

srividya-sundaram · 2024-07-11T20:02:45Z

clang/test/Driver/sycl-device-lib-old-model.cpp

@@ -0,0 +1,233 @@
+///
+/// Perform several driver tests for SYCL device libraries on Linux


Is there a test file for tests using the new offload driver?

For this, no. Any device libraries or objects aren't touched by the driver with the new model, so there aren't any behaviors to verify.

srividya-sundaram · 2024-07-11T20:07:41Z

clang/test/Driver/sycl-device-lib.cpp

@@ -7,227 +7,184 @@
 /// ###########################################################################

 /// test behavior of device library default link and fno-sycl-device-lib-jit-link
-// RUN: %clangxx -fsycl %s --sysroot=%S/Inputs/SYCL -### 2>&1 \
+// RUN: %clangxx -fsycl --offload-new-driver %s --sysroot=%S/Inputs/SYCL -### 2>&1 \


nit - filename can be updated to mention new offload model.

srividya-sundaram · 2024-07-11T20:10:08Z

clang/test/Driver/sycl-device-old-model.cpp

+// CHECK-DEFAULT-NOT: "-fsycl-is-device"
+
+/// Check "-fsycl-is-device" is passed when compiling for device:
+// RUN:   %clang -### -fsycl-device-only %s 2>&1 \


Should --no-offload-new-driver be passed here?

srividya-sundaram · 2024-07-11T20:10:39Z

clang/test/Driver/sycl-device-old-model.cpp

+
+/// Check that "-Wno-sycl-strict" is set on compiler invocation with "-fsycl"
+/// or "-fsycl-device-only" or both:
+// RUN:   %clang -### -fsycl %s 2>&1 \


Same, should --no-offload-new-driver be used here?

srividya-sundaram · 2024-07-11T20:11:11Z

clang/test/Driver/sycl-device-old-model.cpp

+// CHECK-SYCL-AUX-TRIPLE: clang{{.*}} "-aux-triple" "aarch64-unknown-linux-gnu"
+
+/// Verify output files are properly specified given -o
+// RUN: %clang -### -fsycl -fsycl-device-only -o dummy.out %s 2>&1 \


Same question as above.

srividya-sundaram · 2024-07-11T20:11:28Z

clang/test/Driver/sycl-device-old-model.cpp

+// CHECK-SYCL-STD_VERSION: clang{{.*}} "-sycl-std=2020"
+
+/// Check that -aux-triple is set correctly
+// RUN:   %clang -### -fsycl -target aarch64-linux-gnu %s 2>&1 \


Same question.

srividya-sundaram · 2024-07-11T20:14:23Z

clang/test/Driver/sycl-device-optimizations.cpp

@@ -1,11 +1,11 @@
 /// Check that optimizations for sycl device are enabled by default:
-// RUN:   %clang -### -fsycl %s 2>&1 \
+// RUN:   %clang -### -fsycl --offload-new-driver %s 2>&1 \


nit: All test files with the the old driver option have "old model" in their name.
Test files with the new driver model can have their names updated too.

I'd rather not shuffle the names around too much. The idea is that the 'old model' will be going away once we make it the default, so the files renamed as 'old-model' can just be removed (as well as the existing tests that weren't converted to the new offloading model)

I've thought about this a little, and I'm going to rename all of the tests that I'm not converting/adding to the new model to use 'old-model'. This way, we know which ones will go away cleanly (have old-model in the name) and we don't have to worry about the comments.

mdtoguchi · 2024-07-11T21:32:18Z

clang/test/Driver/clang-offload-bundler-bc-archive-support-win.cpp


-#include <sycl/sycl.hpp>
+#include <sycl/sycl.hpp>


All of the non-visual changes are due to a cleanup of some carriage return characters.

mdtoguchi · 2024-07-11T21:32:58Z

clang/test/Driver/clang-offload-bundler-bc-archive-support2-linux.cpp

+// RUN: cmp %t_unbundled.o  `grep .o$  %t_list.txt`
+
+void foo() {
+}


Similar comment - non-visual changes to clean up carriage return characters.

…offload

mdtoguchi · 2024-07-16T18:24:26Z

@intel/llvm-gatekeepers, please take a look, should be ready for merge.

Update existing SYCL based driver tests to use the new offload model and retain the equivalent old offloading model tests be creating copies. These tests use the --offload-new-driver and --no-offload-new-driver options to force a particular model to follow.

mdtoguchi requested a review from a team as a code owner July 11, 2024 16:42

mdtoguchi had a problem deploying to WindowsCILock July 11, 2024 16:44 — with GitHub Actions Error

Fix errors in tests

aab5c24

mdtoguchi had a problem deploying to WindowsCILock July 11, 2024 18:30 — with GitHub Actions Failure

srividya-sundaram reviewed Jul 11, 2024

View reviewed changes

mdtoguchi temporarily deployed to WindowsCILock July 11, 2024 20:20 — with GitHub Actions Inactive

mdtoguchi commented Jul 11, 2024

View reviewed changes

Update tests to add old model switch as needed

d6577f4

mdtoguchi had a problem deploying to WindowsCILock July 11, 2024 21:56 — with GitHub Actions Failure

mdtoguchi had a problem deploying to WindowsCILock July 11, 2024 23:18 — with GitHub Actions Error

Disable test until temp file problem is fixed

87dbfac

mdtoguchi temporarily deployed to WindowsCILock July 11, 2024 23:34 — with GitHub Actions Inactive

mdtoguchi temporarily deployed to WindowsCILock July 12, 2024 00:17 — with GitHub Actions Inactive

mdtoguchi added 2 commits July 12, 2024 08:30

Move tests that are old model only to new old model naming

524cfb9

Merge remote-tracking branch 'intel_llvm/sycl' into update-tests-new-…

a2ab733

…offload

mdtoguchi temporarily deployed to WindowsCILock July 12, 2024 15:42 — with GitHub Actions Inactive

mdtoguchi requested a review from srividya-sundaram July 12, 2024 15:53

srividya-sundaram approved these changes Jul 12, 2024

View reviewed changes

mdtoguchi temporarily deployed to WindowsCILock July 12, 2024 18:06 — with GitHub Actions Inactive

Merge remote-tracking branch 'intel_llvm/sycl' into update-tests-new-…

52e8cac

…offload

mdtoguchi temporarily deployed to WindowsCILock July 15, 2024 20:29 — with GitHub Actions Inactive

mdtoguchi temporarily deployed to WindowsCILock July 15, 2024 21:29 — with GitHub Actions Inactive

sarnex merged commit 7515da8 into intel:sycl Jul 16, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NFC][SYCL] Update tests to reflect new offload model #14539

[NFC][SYCL] Update tests to reflect new offload model #14539

mdtoguchi commented Jul 11, 2024

srividya-sundaram Jul 11, 2024

mdtoguchi Jul 11, 2024

srividya-sundaram Jul 11, 2024

mdtoguchi Jul 11, 2024

srividya-sundaram Jul 11, 2024

mdtoguchi Jul 11, 2024

srividya-sundaram Jul 11, 2024

mdtoguchi Jul 11, 2024

srividya-sundaram Jul 11, 2024

mdtoguchi Jul 11, 2024

srividya-sundaram Jul 11, 2024

srividya-sundaram Jul 11, 2024

srividya-sundaram Jul 11, 2024

srividya-sundaram Jul 11, 2024

srividya-sundaram Jul 11, 2024

srividya-sundaram Jul 11, 2024

mdtoguchi Jul 11, 2024

mdtoguchi Jul 12, 2024

mdtoguchi Jul 11, 2024

mdtoguchi Jul 11, 2024

mdtoguchi commented Jul 16, 2024

		@@ -0,0 +1,233 @@
		///
		/// Perform several driver tests for SYCL device libraries on Linux


		#include <sycl/sycl.hpp>
		#include <sycl/sycl.hpp>

[NFC][SYCL] Update tests to reflect new offload model #14539

[NFC][SYCL] Update tests to reflect new offload model #14539

Conversation

mdtoguchi commented Jul 11, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdtoguchi commented Jul 16, 2024