[SYCL][New offload model] Add SYCL E2E tests for --offload-new-driver option and fix failing tests #14730

asudarsa · 2024-07-23T18:54:02Z

This PR is used to add a set of SYCL E2E tests with --offload-new-driver enabled.
Some failures related to offloading to 3rd party hardware have been resolved.

asudarsa · 2024-07-25T05:20:10Z

Hi @sarnex @maksimsab and @mdtoguchi

Can you please take a look? I have aligned new offloading model flow with old offloading flow for 3rd party hardware.

Thanks

asudarsa · 2024-07-25T06:08:19Z

Hi @Naghasan,

Can you please take a look?

Thanks

sarnex

initial review, thanks!

sarnex · 2024-07-25T14:10:05Z

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

+  CmdArgs.push_back(*TempFileOrErr);
+  CmdArgs.push_back(InputFile);
+  if (Error Err = executeCommands(*PtxasPath, CmdArgs))
+    return std::move(Err);


nit: if we are returning, do we need to std::move?

I see that std::move has been used wherever we are returning Err. I followed the trend.

sarnex · 2024-07-25T14:10:34Z

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

@@ -1272,6 +1313,8 @@ Expected<StringRef> clang(ArrayRef<StringRef> InputFiles, const ArgList &Args) {
  if (!Triple.isNVPTX())
    CmdArgs.push_back("-Wl,--no-undefined");

+  if (IsSYCLKind && Triple.isNVPTX())


do we know why this is SYCL specific and not NVPTX specific?

For now, I am trying to match the behavior of new offloading model with that found in the new offloading model (for SYCL). This particular flow (getting the assembly file from the clang call and then explicitly calling ptxas inside the linker-wrapper tool) matches with what is found in the old offloading model, but does not match the community flow. Hence the SYCL specificity. This surely warrants revisiting at a later time.

sarnex · 2024-07-25T14:11:24Z

clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp

+        if (!ClangOutputOrErr)
+          return ClangOutputOrErr.takeError();
+        if (Triple.isNVPTX()) {
+          auto VirtualArch =


Can we move the NVPTX code inside the if into a separate function?

Is there a specific reason for this change?

Thanks

I thought for the loop was getting a bit complicated

I revisited the code. I agree that the loop is getting a bit complicated. But I am not able to come up with a 'logical' split to move the NVPTX case into a new function.

sarnex · 2024-07-25T14:11:50Z

sycl/test-e2e/NewOffloadDriver/Inputs/split-per-source-second-file.cpp

@@ -0,0 +1,23 @@
+#include "split-per-source.h"


do we need a copyright header?

This file was lifted from here: ./sycl/test-e2e/DeviceCodeSplit/Inputs/split-per-source-second-file.cpp where I do not find a copyright header.

sarnex · 2024-07-25T14:12:17Z

sycl/test-e2e/NewOffloadDriver/aot-cpu.cpp

@@ -0,0 +1,6 @@
+// REQUIRES: opencl-aot, cpu


we also need any-device-is-cpu i think

I am not aware of this setting. But I can add that here. Again this test was lifted from ./sycl/test-e2e/DeviceCodeSplit/aot-cpu.cpp and I added --offload-new-driver

sarnex · 2024-07-25T14:14:57Z

sycl/test-e2e/NewOffloadDriver/cpu.cpp

+//
+//===---------------------------------------------------------------------===//
+
+// REQUIRES: opencl-aot, cpu


need any-device-is-cpu here too i think

Same as above comments. Thanks

sarnex · 2024-07-25T14:16:31Z

sycl/test-e2e/NewOffloadDriver/multisource.cpp

+// Test with `--offload-new-driver`
+// RUN: %{build} --offload-new-driver -c -o %t.kernel.o -DINIT_KERNEL -DCALC_KERNEL
+// RUN: %{build} --offload-new-driver -c -o %t.main.o -DMAIN_APP
+// RUN: %clangxx -fsycl -fsycl-targets=%{sycl_triple} --offload-new-driver %t.kernel.o %t.main.o -o %t.fat


you might be able to use %{build} here

sarnex · 2024-07-25T14:17:48Z

sycl/test-e2e/NewOffloadDriver/split-per-source-main.cpp

+// RUN: --offload-new-driver -fsycl-dead-args-optimization
+// RUN: %{run} %t.out
+//
+// XFAIL: hip_nvidia


do we have a tracker for this XFAIL?

Not sure. I see the PR marking this as XFAIL here: intel/llvm-test-suite#525 But there is no related 'issue' for this.

sarnex · 2024-07-25T14:20:28Z

sycl/test-e2e/NewOffloadDriver/aot-cpu.cpp

+
+// Test with `--offload-new-driver`
+// RUN: %clangxx -fsycl -fsycl-device-code-split=per_source -fsycl-targets=spir64_x86_64 -I %S/Inputs -o %t.out %S/split-per-source-main.cpp %S/Inputs/split-per-source-second-file.cpp \
+// RUN: -fsycl-dead-args-optimization --offload-new-driver


-fsycl-dead-args-optimization seems to be enabled by default, so do we really need to pass it everywhere?

I have lifted this file again from ./sycl/test-e2e/DeviceCodeSplit/aot-cpu.cpp. May be we can have a cleanup PR later on?

mdtoguchi · 2024-07-25T17:00:05Z

clang/lib/Driver/ToolChains/Clang.cpp

@@ -11065,7 +11065,7 @@ void LinkerWrapper::ConstructJob(Compilation &C, const JobAction &JA,
  ArgStringList CmdArgs;

  // Pass the CUDA path to the linker wrapper tool.
-  for (Action::OffloadKind Kind : {Action::OFK_Cuda, Action::OFK_OpenMP}) {
+  for (Action::OffloadKind Kind : {Action::OFK_Cuda, Action::OFK_OpenMP, Action::OFK_SYCL}) {


is there a test we can update to verify that --cuda-path is being passed from the driver?

Good point. Coming soon!!

Added in the latest commit

asudarsa · 2024-07-25T18:19:37Z

Protex IP scan reports are based on 'already' existing tests. It is unclear to me about the action items.

asudarsa · 2024-07-25T18:31:28Z

Hi @sarnex

Thanks so much for the feedback. A quick summary of my responses to your comments about the 'newly' added tests.
All newly added tests have been 'derived' from existing tests with only the addition of '--offload-new-driver' option to use the new offloading model. I agree that the comments about issues in the test need to be addressed. Since this PR did not introduce these issues, I propose that we resolve them in a subsequent PR. I can set up a tracker for that. Hope the resolution is agreeable.

Thanks
Sincerely

sarnex · 2024-07-25T18:32:44Z

Okay with me!

sarnex

lgtm as incremental change, hopefully we can address the cleanup tasks soon

maksimsab

LGTM.

… option Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

…ckend compilation Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

This reverts commit 151909a.

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

maarquitos14 · 2024-07-29T13:16:23Z

@intel/llvm-gatekeepers precommit failure is a known-issue due to hwloc not being available in RHEL. Can we get this merged?

… option and fix failing tests (intel#14730) This PR is used to add a set of SYCL E2E tests with --offload-new-driver enabled. Some failures related to offloading to 3rd party hardware have been resolved. --------- Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa requested a review from a team as a code owner July 23, 2024 18:54

asudarsa marked this pull request as draft July 23, 2024 18:54

asudarsa temporarily deployed to WindowsCILock July 23, 2024 18:55 — with GitHub Actions Inactive

asudarsa had a problem deploying to WindowsCILock July 23, 2024 20:18 — with GitHub Actions Failure

asudarsa had a problem deploying to WindowsCILock July 24, 2024 16:56 — with GitHub Actions Error

asudarsa force-pushed the add_e2e_test_for_new_offload_model branch from 1954f0f to 20e81e2 Compare July 24, 2024 18:41

asudarsa had a problem deploying to WindowsCILock July 24, 2024 18:43 — with GitHub Actions Failure

asudarsa had a problem deploying to WindowsCILock July 24, 2024 19:54 — with GitHub Actions Failure

asudarsa temporarily deployed to WindowsCILock July 25, 2024 05:00 — with GitHub Actions Inactive

asudarsa changed the title ~~[SYCL][New offload model] Add SYCL E2E tests for --offload-new-driver option~~ [SYCL][New offload model] Add SYCL E2E tests for --offload-new-driver option and fix failing tests Jul 25, 2024

asudarsa marked this pull request as ready for review July 25, 2024 05:19

asudarsa requested a review from a team as a code owner July 25, 2024 05:19

asudarsa had a problem deploying to WindowsCILock July 25, 2024 05:33 — with GitHub Actions Failure

sarnex reviewed Jul 25, 2024

View reviewed changes

mdtoguchi reviewed Jul 25, 2024

View reviewed changes

asudarsa force-pushed the add_e2e_test_for_new_offload_model branch from a5fcb73 to 8fdc107 Compare July 25, 2024 18:26

asudarsa requested review from sarnex and mdtoguchi July 25, 2024 18:34

sarnex approved these changes Jul 25, 2024

View reviewed changes

mdtoguchi approved these changes Jul 25, 2024

View reviewed changes

asudarsa temporarily deployed to WindowsCILock July 25, 2024 18:47 — with GitHub Actions Inactive

asudarsa had a problem deploying to WindowsCILock July 25, 2024 21:03 — with GitHub Actions Failure

asudarsa force-pushed the add_e2e_test_for_new_offload_model branch from 47c3185 to 97fbc98 Compare July 26, 2024 03:41

asudarsa temporarily deployed to WindowsCILock July 26, 2024 03:47 — with GitHub Actions Inactive

asudarsa had a problem deploying to WindowsCILock July 26, 2024 04:24 — with GitHub Actions Failure

maarquitos14 temporarily deployed to WindowsCILock July 26, 2024 09:43 — with GitHub Actions Inactive

maarquitos14 had a problem deploying to WindowsCILock July 26, 2024 10:25 — with GitHub Actions Failure

maksimsab approved these changes Jul 26, 2024

View reviewed changes

asudarsa added 9 commits July 28, 2024 09:25

[SYCL][New offload model] Add SYCL E2E tests for --offload-new-driver…

7c1b9ca

… option Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

[NVPTX][New offload model] Fix clang-linker-wrapper flow for NVPTX ba…

3cb7a33

…ckend compilation Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Fix -g issue with ptxas

88f964d

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Fix -g issue with ptxas

f053bd5

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Fix test fails

b02cfd0

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Add test for --cuda-path passing to clang-linker-wrapper

d667c63

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Fix format issues

91c3fae

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Fix test after merge

0c1af79

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Fix windows fail

151909a

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa force-pushed the add_e2e_test_for_new_offload_model branch from 2b6e88d to 151909a Compare July 28, 2024 16:25

asudarsa had a problem deploying to WindowsCILock July 28, 2024 16:25 — with GitHub Actions Failure

asudarsa had a problem deploying to WindowsCILock July 28, 2024 17:01 — with GitHub Actions Failure

asudarsa added 3 commits July 29, 2024 03:07

Revert "Fix windows fail"

5d728c7

This reverts commit 151909a.

Remove failing test; will be fixed later

a96e739

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

Modernize test

f720222

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa temporarily deployed to WindowsCILock July 29, 2024 10:12 — with GitHub Actions Inactive

asudarsa had a problem deploying to WindowsCILock July 29, 2024 10:46 — with GitHub Actions Error

formatting change

a858ce5

Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

asudarsa temporarily deployed to WindowsCILock July 29, 2024 11:21 — with GitHub Actions Inactive

asudarsa temporarily deployed to WindowsCILock July 29, 2024 11:56 — with GitHub Actions Inactive

dm-vodopyanov merged commit 1de77c3 into intel:sycl Jul 29, 2024
14 of 15 checks passed

[SYCL][New offload model] Add SYCL E2E tests for --offload-new-driver option and fix failing tests #14730

[SYCL][New offload model] Add SYCL E2E tests for --offload-new-driver option and fix failing tests #14730

Uh oh!

Conversation

asudarsa commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asudarsa commented Jul 25, 2024

Uh oh!

asudarsa commented Jul 25, 2024

Uh oh!

sarnex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asudarsa Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asudarsa commented Jul 25, 2024

Uh oh!

asudarsa commented Jul 25, 2024

Uh oh!

sarnex commented Jul 25, 2024

Uh oh!

sarnex left a comment

Choose a reason for hiding this comment

Uh oh!

maksimsab left a comment

Choose a reason for hiding this comment

Uh oh!

maarquitos14 commented Jul 29, 2024

Uh oh!

Uh oh!

Uh oh!

asudarsa commented Jul 23, 2024 •

edited

Loading

asudarsa Jul 25, 2024 •

edited

Loading