Reintroduce reverted commit "[clang-offload-bundler] Standardize TargetID field for bundler #9631

asudarsa · 2023-05-28T03:04:55Z

This commit reintroduces the following LLVM open-source PR: https://reviews.llvm.org/D145770
Some changes on top of the community change have been made as well.

Thanks

…etID field for bundler" This commit reintroduces the following LLVM open-source PR: https://reviews.llvm.org/D145770 Some changes on top of the community change have been made as well. Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa · 2023-05-28T03:06:06Z

Keeping this in draft mode till testing passes. Thanks

Signed-off-by: Arvind Sudarsanam <[email protected]>

…ssue

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa · 2023-06-09T03:34:49Z

Two linux failures are unrelated to this PR.
Issues:
#9804
#9766

Investigating the windows failure. Previously the test was marked with
// REQUIRES: powerpc-registered-target which would have prevented this test from running on our x86 targets. So, this failure might have been present without the proposed change as well.

Will investigate further.

Thanks

Signed-off-by: Arvind Sudarsanam <[email protected]>

mdtoguchi · 2023-06-09T16:15:17Z

clang/test/Driver/clang-offload-bundler-old-bundle-with-new-bundler.c

@@ -0,0 +1,10 @@
+// REQUIRES: x86-registered-target
+// UNSUPPORTED: target={{.*}}-darwin{{.*}}, target={{.*}}-aix{{.*}}, system-windows


Why won't this test work for Windows?

As mentioned offline, this test was never running in the past. There are some issues in the way temporary files are generated that causes failure in windows. It is beyond the scope of this change and should be addressed as a separate issue.

Thanks

mdtoguchi · 2023-06-09T16:21:42Z

clang/lib/Driver/Driver.cpp

@@ -5981,7 +5991,7 @@ class OffloadingActionBuilder final {
          SectionTriple += "-";
          SectionTriple += SyclTarget.BoundArch;
        }
-
+        SectionTriple = standardizedTriple(SectionTriple);


If the bundler is doing the 'standardizing' to determine if the bundled sections are there - why do we need to do any standardizing on the caller side?

Here, we are not calling the bundler. We seem to be reading the sections created by bundler directly.

Thanks

Ah, I see - so here we are scanning all of the objects/archives and retrieving the section names. These names could be names that are created with the updated standardized bundler behavior, requiring us to fix up any names for comparisons on the Driver end.

asudarsa · 2023-06-15T20:16:12Z

@intel/dpcpp-tools-reviewers

Friendly ping.

Thanks

clang/lib/Driver/Driver.cpp

clang/tools/clang-offload-deps/ClangOffloadDeps.cpp

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa · 2023-06-19T22:41:36Z

Latest change only added comments. Test failures do not seem to be related to my changes.

Thanks

maksimsab

LGTM.

sarnex

Discussed ESIMD emu test failures with Arvind offline, they are definitely not related to this PR and seem to be a CI issue. These tests should be disabled on the emulator in current HEAD. I didn't review the rest of the change.

asudarsa · 2023-06-27T14:53:54Z

Hi @AlexeySachkov

I have looked at all the failures and none of them are related to my PR. Can you please take a look at this PR and merge if appropriate?

Thanks
Sincerely

AlexeySachkov · 2023-06-29T12:18:37Z

Hi @AlexeySachkov

I have looked at all the failures and none of them are related to my PR. Can you please take a look at this PR and merge if appropriate?

Thanks Sincerely

I'm a bit hesitant to merge the PR with so many failed tests. Let's take a look at newer results first to make sure that those failures are indeed unrelated

AlexeySachkov · 2023-06-30T16:22:20Z

Fails:

********************
Failed Tests (1):
  SYCL :: ESIMD/accessor_local.cpp

Covered by #10138

********************
Failed Tests (4):
  SYCL :: USM/memops2d/copy2d_device_to_host.cpp
  SYCL :: USM/memops2d/copy2d_host_to_device.cpp
  SYCL :: USM/memops2d/memcpy2d_device_to_host.cpp
  SYCL :: USM/memops2d/memcpy2d_host_to_device.cpp

Covered by #10157

steffenlarsen · 2023-07-04T09:33:13Z

@asudarsa - clang/test/Driver/clang-offload-bundler-standardize.c is failing in post-commit after this change. Could you please address this ASAP?

dm-vodopyanov · 2023-07-06T10:51:16Z

@asudarsa ping, the post-commit is red for 6 days because of this.

asudarsa · 2023-07-06T15:27:24Z

Looking at this now. It looks mostly like a issue in options being handled by driver.
Thanks.

This test fails in post-commit. Issue: #10238

This patch fixes small issues after #9631 When standardizing the triples the format should be: ``` <arch><sub>-<vendor>-<sys>-<env> ``` * https://clang.llvm.org/docs/CrossCompilation.html#target-triple And then we add our offload arch afterwards, so the final triples should look something like: `nvptx64-nvidia-cuda--sm_50`, instead of `nvptx64-nvidia-cuda-sm_50-`. In addition never skip standardization so that triples like `nvptx64-nvidia-cuda-`, are also properly standardized to `nvptx64-nvidia-cuda--`.

…ize TargetID field for bundler (intel#9631)" This reverts commit d10a290.

…ad-bundler] Standardize TargetID field for bundler (#9631)"" (#10950) This reverts commit d10a290. When the compiler encounters library files built with older compiler, it is not able to understand the old triple and hence fails to grab the library files. This issue needs to be root-caused before trying to submit this again. Thanks --------- Signed-off-by: Sudarsanam, Arvind <[email protected]>

This patch fixes small issues after intel#9631 When standardizing the triples the format should be: ``` <arch><sub>-<vendor>-<sys>-<env> ``` * https://clang.llvm.org/docs/CrossCompilation.html#target-triple And then we add our offload arch afterwards, so the final triples should look something like: `nvptx64-nvidia-cuda--sm_50`, instead of `nvptx64-nvidia-cuda-sm_50-`. In addition never skip standardization so that triples like `nvptx64-nvidia-cuda-`, are also properly standardized to `nvptx64-nvidia-cuda--`.

asudarsa requested review from a team as code owners May 28, 2023 03:04

asudarsa marked this pull request as draft May 28, 2023 03:05

asudarsa temporarily deployed to aws May 28, 2023 03:25 — with GitHub Actions Inactive

Fix type

39ba794

asudarsa temporarily deployed to aws May 28, 2023 03:46 — with GitHub Actions Inactive

asudarsa added 2 commits May 27, 2023 22:47

Fix test fail

3e5e586

Signed-off-by: Arvind Sudarsanam <[email protected]>

Format fix

60af2b0

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa temporarily deployed to aws May 28, 2023 13:59 — with GitHub Actions Inactive

Fix test fails

65a7ae1

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa temporarily deployed to aws May 28, 2023 15:11 — with GitHub Actions Inactive

asudarsa temporarily deployed to aws May 28, 2023 15:49 — with GitHub Actions Inactive

Fix windows test fail

19be7ab

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa temporarily deployed to aws May 28, 2023 16:20 — with GitHub Actions Inactive

asudarsa temporarily deployed to aws May 28, 2023 16:58 — with GitHub Actions Inactive

asudarsa added 3 commits June 5, 2023 09:03

Merge remote-tracking branch 'intel_llvm/sycl' into fix_bundler_std_i…

554e033

…ssue

Fix clang-offload-deps

184c9e8

Signed-off-by: Arvind Sudarsanam <[email protected]>

Add a comment

02b0933

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa temporarily deployed to aws June 9, 2023 00:38 — with GitHub Actions Inactive

asudarsa temporarily deployed to aws June 9, 2023 01:35 — with GitHub Actions Inactive

asudarsa mentioned this pull request Jun 9, 2023

Test SYCL :: KernelAndProgram/kernel-bundle-merge-options-env.cpp failed with Linux SYCL-E2E testing #9804

Open

turning off test for windows temporarily

04a6d5e

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa marked this pull request as ready for review June 9, 2023 14:19

asudarsa temporarily deployed to aws June 9, 2023 14:43 — with GitHub Actions Inactive

asudarsa temporarily deployed to aws June 9, 2023 16:18 — with GitHub Actions Inactive

mdtoguchi reviewed Jun 9, 2023

View reviewed changes

asudarsa requested a review from mdtoguchi June 15, 2023 20:15

mdtoguchi approved these changes Jun 15, 2023

View reviewed changes

maksimsab requested changes Jun 16, 2023

View reviewed changes

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

clang/tools/clang-offload-deps/ClangOffloadDeps.cpp Show resolved Hide resolved

Updated comments

d44e71a

Signed-off-by: Arvind Sudarsanam <[email protected]>

asudarsa requested a review from maksimsab June 19, 2023 19:36

asudarsa temporarily deployed to aws June 19, 2023 19:52 — with GitHub Actions Inactive

asudarsa temporarily deployed to aws June 19, 2023 20:31 — with GitHub Actions Inactive

maksimsab approved these changes Jun 21, 2023

View reviewed changes

sarnex approved these changes Jun 27, 2023

View reviewed changes

Merge remote-tracking branch 'origin/sycl' into fix_bundler_std_issue

b281004

AlexeySachkov temporarily deployed to aws June 29, 2023 12:47 — with GitHub Actions Inactive

AlexeySachkov temporarily deployed to aws June 29, 2023 15:13 — with GitHub Actions Inactive

AlexeySachkov merged commit d10a290 into intel:sycl Jun 30, 2023

dm-vodopyanov mentioned this pull request Jul 6, 2023

Failure in Clang :: Driver/clang-offload-bundler-standardize.c in post-commit #10238

Closed

dm-vodopyanov added a commit that referenced this pull request Jul 6, 2023

[SYCL] Disable clang-offload-bundler-standardize.c after #9631 (#10240)

a238f3d

This test fails in post-commit. Issue: #10238

Chenyang-L mentioned this pull request Jul 11, 2023

LLVM and SPIRV-LLVM-Translator pulldown (WW25) #10311

Closed

npmiller mentioned this pull request Jul 14, 2023

[SYCL] Fix triple in offload mismatch warning #10385

Merged

asudarsa added a commit to asudarsa/llvm-2 that referenced this pull request Aug 23, 2023

Revert "Reintroduce reverted commit "[clang-offload-bundler] Standard…

e478c32

…ize TargetID field for bundler (intel#9631)" This reverts commit d10a290.

AlexeySachkov mentioned this pull request Jan 12, 2024

[SYCL][Doc] Add Nov'23 release notes #12365

Merged

		@@ -0,0 +1,10 @@
		// REQUIRES: x86-registered-target
		// UNSUPPORTED: target={{.}}-darwin{{.}}, target={{.}}-aix{{.}}, system-windows

Reintroduce reverted commit "[clang-offload-bundler] Standardize TargetID field for bundler #9631

Reintroduce reverted commit "[clang-offload-bundler] Standardize TargetID field for bundler #9631

Uh oh!

Conversation

asudarsa commented May 28, 2023

Uh oh!

asudarsa commented May 28, 2023

Uh oh!

asudarsa commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdtoguchi Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

asudarsa Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

mdtoguchi Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

asudarsa Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

mdtoguchi Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

asudarsa commented Jun 15, 2023

Uh oh!

Uh oh!

Uh oh!

asudarsa commented Jun 19, 2023

Uh oh!

maksimsab left a comment

Choose a reason for hiding this comment

Uh oh!

sarnex left a comment • 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!

asudarsa commented Jun 27, 2023

Uh oh!

AlexeySachkov commented Jun 29, 2023

Uh oh!

AlexeySachkov commented Jun 30, 2023

Uh oh!

steffenlarsen commented Jul 4, 2023

Uh oh!

dm-vodopyanov commented Jul 6, 2023

Uh oh!

asudarsa commented Jul 6, 2023

Uh oh!

Uh oh!

asudarsa commented Jun 9, 2023 •

edited

Loading

sarnex left a comment •

edited

Loading