[NVPTX][AMD][New offload model] Add support for -fsycl-embed-ir in the new offloading model #14526

asudarsa · 2024-07-11T05:05:13Z

When compiling for Nvidia/AMD devices and the user requested the IR to be embedded in the application (via option), We run the output of sycl-post-link (filetable referencing LLVM Bitcode + symbols) through the offload wrapper and link the resulting object to the application.

asudarsa · 2024-07-11T05:06:46Z

There will be merge issues with a parallel PR. #13806
Depending on which one gets merged, the other PR will need to be adjusted a bit.

Thanks

clang/test/Driver/sycl-embed-ir-new-offload.cpp

sarnex

lgtm, minor comments!

sarnex · 2024-07-11T19:43:08Z

sycl/test-e2e/KernelFusion/diamond_shape_new_driver.cpp

@@ -0,0 +1,106 @@
+// RUN: %{build} %{embed-ir} -O2 --offload-new-driver -o %t.out


Can we put this in the sycl/test-e2e/NewOffloadDriver folder instead?

What is the plan for these tests after the new offload driver becomes the default?

If the plan is to delete the tests using the new driver explicitly after the default behavior is changed, I agree with @sarnex that it would make sense to put them all in the same place.

hi Lukas and Nick

Thanks so much for feedback here.
I am going to remove the E2E test here. I have a new PR coming in very soon that adds a sequence of test for new driver and I will include this there.
I evaluated three options:

Adding a new file in the same location as the original test

Adding a new file in a separate folder

Adding few lines in existing test to enable new offload testing

It seems to me that Option #3 will be the easiest to maintain when we make changes to the test in the future.
@sarnex and @sommerlukas WDYT?

Thanks

IMO 2) is the easiest to maintain, they're all in the same place and won't bog down teams not working on tools

Hi @sarnex . I thought about that. But, users making changes to the original test should remember to change the NewOffloadModel test as well. I thought that was a painpoint.

I don't feel super strongly on this, so if the test owners are okay with it, it's fine with me.

I don't feel strong about the location, but I'd prefer if we had a test actually using the IR embedded by the flag.

For example, I think that none of the tests currently in this PR check whether the embedded IR has the correct target name/prefix as expected by the SYCL runtime.

With an e2e test, we would ensure that IR embedding is actually functional.

Option 3 would also be viable to achieve that.

I am adding the test back. Based on further 'internal thinking' and the fact that there is no 'strong' push for a particular option , I have decided to go with option 2. This will atleast avoid triaging headaches as @sarnex suggested.

Thanks again for discussion.

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

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

clang/test/Driver/sycl-offload-new-driver.c

+/// check for -sycl-embed-ir transmission to clang-linker-wrapper tool
+// RUN: %clangxx -fsycl -### -fsycl-targets=nvptx64-nvidia-cuda \
+// RUN:          -fno-sycl-libspirv -nocudalib --offload-new-driver \
+// RUN:          -fsycl-embed-ir %s 2>&1 \


What happens if -fsycl-embed-ir is passed for devices other than NVidia or AMD?

According to our implementation, it will be ignored. I can add a test for it.

Is that acceptable? Would a warning or error be appropriate here?

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

sommerlukas · 2024-07-12T08:04:21Z

sycl/test-e2e/KernelFusion/diamond_shape_new_driver.cpp

@@ -0,0 +1,106 @@
+// RUN: %{build} %{embed-ir} -O2 --offload-new-driver -o %t.out


What is the plan for these tests after the new offload driver becomes the default?

If the plan is to delete the tests using the new driver explicitly after the default behavior is changed, I agree with @sarnex that it would make sense to put them all in the same place.

mdtoguchi

Looks OK to me.

asudarsa · 2024-07-12T17:23:28Z

Removed the 'ccc-print-phases' testing based on comment from @mdtoguchi

Thanks

…e new offloading model Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

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

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

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

clang/lib/Driver/ToolChains/Clang.cpp

KseniyaTikhomirova

LGTM

sycl/test-e2e/NewOffloadDriver/diamond_shape.cpp

maksimsab · 2024-07-24T15:39:21Z

The following test is unrelated to the current PR since it fails in others PRs.
SYCL :: EnqueueNativeCommand/custom-command-multiple-dev-cuda.cpp

maksimsab · 2024-07-25T11:33:38Z

@intel/llvm-gatekeepers Can we merge it?

sommerlukas · 2024-07-25T12:08:22Z

Unrelated failure of CUDA CI is tracked in #14715.

…e new offloading model (intel#14526) When compiling for Nvidia/AMD devices and the user requested the IR to be embedded in the application (via option), We run the output of sycl-post-link (filetable referencing LLVM Bitcode + symbols) through the offload wrapper and link the resulting object to the application. --------- Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com> Co-authored-by: Sabianin, Maksim <maksim.sabianin@intel.com>

asudarsa requested review from a team as code owners July 11, 2024 05:05

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

asudarsa temporarily deployed to WindowsCILock July 11, 2024 05:43 — with GitHub Actions Inactive

asudarsa temporarily deployed to WindowsCILock July 11, 2024 13:03 — with GitHub Actions Inactive

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

mdtoguchi reviewed Jul 11, 2024

View reviewed changes

clang/test/Driver/sycl-embed-ir-new-offload.cpp Outdated Show resolved Hide resolved

mdtoguchi reviewed Jul 11, 2024

View reviewed changes

clang/test/Driver/sycl-embed-ir-new-offload.cpp Outdated Show resolved Hide resolved

sarnex reviewed Jul 11, 2024

View reviewed changes

srividya-sundaram reviewed Jul 11, 2024

View reviewed changes

sommerlukas reviewed Jul 12, 2024

View reviewed changes

asudarsa requested review from sarnex, sommerlukas, srividya-sundaram and mdtoguchi July 12, 2024 14:14

asudarsa temporarily deployed to WindowsCILock July 12, 2024 14:15 — with GitHub Actions Inactive

sarnex approved these changes Jul 12, 2024

View reviewed changes

asudarsa temporarily deployed to WindowsCILock July 12, 2024 15:14 — with GitHub Actions Inactive

mdtoguchi approved these changes Jul 12, 2024

View reviewed changes

asudarsa requested a review from a team as a code owner July 12, 2024 17:24

asudarsa requested a review from KseniyaTikhomirova July 12, 2024 17:24

asudarsa had a problem deploying to WindowsCILock July 12, 2024 17:25 — with GitHub Actions Error

asudarsa added 4 commits July 12, 2024 10:25

[NVPTX][AMD][New offload model] Add support for -fsycl-embed-ir in th…

fa8e908

…e new offloading model Signed-off-by: Arvind Sudarsanam <arvind.sudarsanam@intel.com>

add more tests

2b1c1ea

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

Address review comments

96e3e04

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

Adding back E2E test and then removing a driver test that is not requ…

e34ae78

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

asudarsa force-pushed the add_sycl_embed_ir_support_new_offload branch from 9c76275 to e34ae78 Compare July 12, 2024 17:26

asudarsa temporarily deployed to WindowsCILock July 12, 2024 17:27 — with GitHub Actions Inactive

srividya-sundaram approved these changes Jul 12, 2024

View reviewed changes

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

Add local cfg file

6440883

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

asudarsa temporarily deployed to WindowsCILock July 12, 2024 23:15 — with GitHub Actions Inactive

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

sommerlukas approved these changes Jul 15, 2024

View reviewed changes

Naghasan reviewed Jul 15, 2024

View reviewed changes

clang/lib/Driver/ToolChains/Clang.cpp Show resolved Hide resolved

KseniyaTikhomirova approved these changes Jul 16, 2024

View reviewed changes

sycl/test-e2e/NewOffloadDriver/diamond_shape.cpp Show resolved Hide resolved

maksimsab added 2 commits July 24, 2024 06:32

Merge branch 'sycl' into add_sycl_embed_ir_support_new_offload

fb26813

add restriction in diamond_shape.cpp test

1b93db8

maksimsab had a problem deploying to WindowsCILock July 24, 2024 14:25 — with GitHub Actions Error

Merge branch 'sycl' into add_sycl_embed_ir_support_new_offload

a70df22

maksimsab temporarily deployed to WindowsCILock July 24, 2024 14:28 — with GitHub Actions Inactive

maksimsab temporarily deployed to WindowsCILock July 24, 2024 15:17 — with GitHub Actions Inactive

sommerlukas merged commit 676e851 into intel:sycl Jul 25, 2024
14 of 15 checks passed

dm-vodopyanov mentioned this pull request Jul 25, 2024

[SYCL] Rename sycl-fusion to sycl-jit #14762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NVPTX][AMD][New offload model] Add support for -fsycl-embed-ir in the new offloading model #14526

[NVPTX][AMD][New offload model] Add support for -fsycl-embed-ir in the new offloading model #14526

asudarsa commented Jul 11, 2024

asudarsa commented Jul 11, 2024

sarnex left a comment

sarnex Jul 11, 2024

sommerlukas Jul 12, 2024

asudarsa Jul 12, 2024

sarnex Jul 12, 2024

asudarsa Jul 12, 2024

sarnex Jul 12, 2024

sommerlukas Jul 12, 2024

asudarsa Jul 12, 2024 •

edited

Loading

srividya-sundaram Jul 11, 2024

asudarsa Jul 12, 2024

srividya-sundaram Jul 12, 2024

sommerlukas Jul 12, 2024

mdtoguchi left a comment

asudarsa commented Jul 12, 2024

KseniyaTikhomirova left a comment

maksimsab commented Jul 24, 2024

maksimsab commented Jul 25, 2024

sommerlukas commented Jul 25, 2024

		@@ -0,0 +1,106 @@
		// RUN: %{build} %{embed-ir} -O2 --offload-new-driver -o %t.out

[NVPTX][AMD][New offload model] Add support for -fsycl-embed-ir in the new offloading model #14526

[NVPTX][AMD][New offload model] Add support for -fsycl-embed-ir in the new offloading model #14526

Conversation

asudarsa commented Jul 11, 2024

asudarsa commented Jul 11, 2024

sarnex 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudarsa Jul 12, 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

Choose a reason for hiding this comment

mdtoguchi left a comment

Choose a reason for hiding this comment

asudarsa commented Jul 12, 2024

KseniyaTikhomirova left a comment

Choose a reason for hiding this comment

maksimsab commented Jul 24, 2024

maksimsab commented Jul 25, 2024

sommerlukas commented Jul 25, 2024

asudarsa Jul 12, 2024 •

edited

Loading