[libclc][Cmake] Change libclc library output library to clang resource dir #20701

wenju-he · 2025-11-20T11:36:09Z

Restore pulldown omissions:

Restore output layout from df74736: build/lib/clc -> build/lib/clang/<LLVM_VERSION>/lib/libclc
Restore standalone build LIBCLC_INSTALL_DIR from b9e2f7a.

Additional changes:

Remove repeated directory creation for LIBCLC_OUTPUT_LIBRARY_DIR.
Install libspirv remangled-* bitcodes into LIBCLC_INSTALL_DIR.
SYCLInstallationDetector findLibspirvPath is now simplifed.
Updated libspirv path in sycl-jit/jit-compiler.

…e dir Restore pulldown omissions: * Restore output layout from df74736: build/lib/clc -> build/lib/clang/<LLVM_VERSION>/lib/libclc * Restore standalone build LIBCLC_INSTALL_DIR from b9e2f7a. Additional changes: * Remove repeated directory creation for LIBCLC_OUTPUT_LIBRARY_DIR. * Install libspirv remangled-* bitcodes into LIBCLC_INSTALL_DIR. * SYCLInstallationDetector findLibspirvPath is now simplifed.

Maetveis

Driver and driver test changes LGTM with some nits. I haven't checked CMake changes, that's mostly coming from upstream right?

clang/lib/Driver/ToolChains/SYCL.cpp

clang/test/Driver/sycl-libspirv-toolchain.cpp

Maetveis · 2025-11-24T10:25:00Z

clang/test/Driver/sycl-libspirv-toolchain.cpp

-// RUN: | FileCheck %s --check-prefixes=CHECK-DEVICE-ONLY
-// CHECK-DEVICE-ONLY: "-cc1"{{.*}} "-fsycl-is-device"{{.*}} "-mlink-builtin-bitcode" "{{.*[\\/]}}remangled-{{.*}}.libspirv-nvptx64-nvidia-cuda.bc"
+// RUN: %clang -### -resource-dir %{resource_dir} -fsycl -fsycl-targets=nvptx64-nvidia-cuda -nocudalib -target x86_64-unknown-windows-msvc %s 2>&1 \
+// RUN: | FileCheck %s -DRESOURCE_DIR=%{resource_dir} --check-prefixes=CHECK-WINDOWS


Keep the FileCheck and CHECK lines here the same as they were. These checks are to verify the base name of the library, it's better to keep them independent of the absolute path.

Keep the FileCheck and CHECK lines here the same as they were. These checks are to verify the base name of the library, it's better to keep them independent of the absolute path.

done in 4f560f8

My comment applies to all checks in that block, I marked all of them now.

This reverts commit 8da81b7.

Co-authored-by: Mészáros Gergely <maetveis@gmail.com>

wenju-he · 2025-11-25T07:48:57Z

I haven't checked CMake changes, that's mostly coming from upstream right?

Yes, except for db7fc8a and change to remangled libspirv install.

Maetveis · 2025-11-25T15:06:09Z

clang/test/Driver/sycl-libspirv-toolchain.cpp

-// RUN: | FileCheck %s --check-prefixes=CHECK-LINUX
-// CHECK-LINUX: "-cc1"{{.*}} "-fsycl-is-device"{{.*}} "-mlink-builtin-bitcode" "{{.*[\\/]}}remangled-l64-signed_char.libspirv-nvptx64-nvidia-cuda.bc"
+// RUN: %clang -### -resource-dir %{resource_dir} -fsycl -fsycl-targets=nvptx64-nvidia-cuda -nocudalib -target x86_64-unknown-linux-gnu %s 2>&1 \
+// RUN: | FileCheck %s -DRESOURCE_DIR=%{resource_dir} --check-prefixes=CHECK-LINUX


here too the same as in #20701 (comment)

here too the same as in #20701 (comment)

done

Maetveis · 2025-11-25T15:06:41Z

clang/test/Driver/sycl-libspirv-toolchain.cpp

+// RUN: %clang -### -resource-dir %{resource_dir} -fsycl -fsycl-targets=nvptx64-nvidia-cuda -nocudalib -target x86_64-unknown-linux-gnu %s 2>&1 \
+// RUN: | FileCheck %s -DRESOURCE_DIR=%{resource_dir} --check-prefixes=CHECK-LINUX
+// RUN: %clang -### -resource-dir %{resource_dir} -fsycl -fsycl-targets=nvptx64-nvidia-cuda -nocudalib -target x86_64-unknown-windows-cygnus %s 2>&1 \
+// RUN: | FileCheck %s -DRESOURCE_DIR=%{resource_dir} --check-prefixes=CHECK-LINUX


here too the same as in #20701 (comment)

here too the same as in #20701 (comment)

done

Maetveis · 2025-11-25T15:06:50Z

clang/test/Driver/sycl-libspirv-toolchain.cpp

-// RUN: | FileCheck %s --check-prefixes=CHECK-AMDGCN-WINDOWS
-// CHECK-AMDGCN-WINDOWS: "-cc1"{{.*}} "-fsycl-is-device"{{.*}} "-mlink-builtin-bitcode" "{{.*[\\/]}}remangled-l32-signed_char.libspirv-amdgcn-amd-amdhsa.bc"
+// RUN: %clang -### -resource-dir %{resource_dir} -fsycl -fsycl-targets=amdgcn-amd-amdhsa -Xsycl-target-backend --offload-arch=gfx908 -nogpulib -target x86_64-unknown-windows-msvc %s 2>&1 \
+// RUN: | FileCheck %s -DRESOURCE_DIR=%{resource_dir} --check-prefixes=CHECK-AMDGCN-WINDOWS


here too the same as in #20701 (comment)

here too the same as in #20701 (comment)

done

Maetveis · 2025-11-25T15:07:00Z

clang/test/Driver/sycl-libspirv-toolchain.cpp

-// RUN: | FileCheck %s --check-prefixes=CHECK-DEVICE-ONLY
-// CHECK-DEVICE-ONLY: "-cc1"{{.*}} "-fsycl-is-device"{{.*}} "-mlink-builtin-bitcode" "{{.*[\\/]}}remangled-{{.*}}.libspirv-nvptx64-nvidia-cuda.bc"
+// RUN: %clang -### -resource-dir %{resource_dir} -fsycl -fsycl-device-only -fsycl-targets=nvptx64-nvidia-cuda -nocudalib %s 2>&1 \
+// RUN: | FileCheck %s -DRESOURCE_DIR=%{resource_dir} --check-prefixes=CHECK-DEVICE-ONLY


here too the same as in #20701 (comment)

here too the same as in #20701 (comment)

done

aelovikov-intel · 2025-11-25T16:06:39Z

sycl-jit/jit-compiler/lib/rtc/DeviceCompilation.cpp

+  Driver D{(SYCLToolchain::instance().getPrefix() + "/bin/clang++").str(),
+           T.getTriple(), Diags};


There is getClangXXExe:

llvm/sycl-jit/jit-compiler/lib/rtc/DeviceCompilation.cpp

Lines 616 to 617 in e711ac0

std::string_view getPrefix() const { return Prefix; }

std::string_view getClangXXExe() const { return ClangXXExe; }

I understand this is a move, but we can update it still.

There is getClangXXExe:

llvm/sycl-jit/jit-compiler/lib/rtc/DeviceCompilation.cpp

Lines 616 to 617 in e711ac0

std::string_view getPrefix() const { return Prefix; }

std::string_view getClangXXExe() const { return ClangXXExe; }

I understand this is a move, but we can update it still.

done, updated to use getClangXXExe

aelovikov-intel · 2025-11-25T16:12:31Z

sycl-jit/jit-compiler/lib/rtc/DeviceCompilation.cpp

-        (SYCLToolchain::instance().getPrefix() + "/lib/" + LibName).str();
+    std::string LibPath;
+    if (LibName.find("libspirv") != std::string::npos) {
+      SmallString<256> LibraryPath(D.ResourceDir);


IMO, resource dir getter should go to the SYCLToolchain class, maybe it should be even pre-computed and stored as a data member.

IMO, resource dir getter should go to the SYCLToolchain class, maybe it should be even pre-computed and stored as a data member.

done, moved to a data member of SYCLToolchain

aelovikov-intel · 2025-11-25T16:14:05Z

sycl-jit/jit-compiler/lib/rtc/DeviceCompilation.cpp

+    std::string LibPath;
+    if (LibName.find("libspirv") != std::string::npos) {
+      SmallString<256> LibraryPath(D.ResourceDir);
+      sys::path::append(LibraryPath, "lib", "libclc", LibName);


I don't think we use sys::path::append in this file (probably erroneously), but can we use simple string concatenation for uniformity?

I don't think we use sys::path::append in this file (probably erroneously), but can we use simple string concatenation for uniformity?

done

aelovikov-intel

sycl-jit LGTM, thanks!

wenju-he requested review from a team as code owners November 20, 2025 11:36

wenju-he requested review from Maetveis and bratpiorka November 20, 2025 11:36

wenju-he had a problem deploying to WindowsCILock November 20, 2025 11:36 — with GitHub Actions Error

clang-format

2631041

wenju-he had a problem deploying to WindowsCILock November 20, 2025 11:48 — with GitHub Actions Failure

wenju-he had a problem deploying to WindowsCILock November 20, 2025 12:22 — with GitHub Actions Failure

fix check-libclc

f21453f

Maetveis approved these changes Nov 24, 2025

View reviewed changes

restore libspirv install component

db7fc8a

wenju-he temporarily deployed to WindowsCILock November 24, 2025 12:48 — with GitHub Actions Inactive

wenju-he temporarily deployed to WindowsCILock November 24, 2025 13:19 — with GitHub Actions Inactive

add search path for sycl-jit-toolchain

8da81b7

wenju-he had a problem deploying to WindowsCILock November 25, 2025 03:02 — with GitHub Actions Failure

wenju-he temporarily deployed to WindowsCILock November 25, 2025 03:37 — with GitHub Actions Inactive

wenju-he had a problem deploying to WindowsCILock November 25, 2025 03:37 — with GitHub Actions Failure

wenju-he added 2 commits November 25, 2025 06:58

Revert "add search path for sycl-jit-toolchain"

b2ef5a2

This reverts commit 8da81b7.

fix search path for sycl-jit-toolchain

7bfffff

wenju-he requested review from a team, aelovikov-intel and cperkinsintel as code owners November 25, 2025 07:11

wenju-he temporarily deployed to WindowsCILock November 25, 2025 07:12 — with GitHub Actions Inactive

update per review comments

4f560f8

wenju-he had a problem deploying to WindowsCILock November 25, 2025 07:48 — with GitHub Actions Error

Update clang/test/Driver/sycl-libspirv-toolchain.cpp

7693b23

Co-authored-by: Mészáros Gergely <maetveis@gmail.com>

wenju-he temporarily deployed to WindowsCILock November 25, 2025 07:49 — with GitHub Actions Inactive

wenju-he temporarily deployed to WindowsCILock November 25, 2025 08:24 — with GitHub Actions Inactive

bratpiorka approved these changes Nov 25, 2025

View reviewed changes

use D.ResourceDir in jit_compiler::linkDeviceLibraries

5a5755d

wenju-he had a problem deploying to WindowsCILock November 25, 2025 12:06 — with GitHub Actions Failure

wenju-he temporarily deployed to WindowsCILock November 25, 2025 12:45 — with GitHub Actions Inactive

wenju-he had a problem deploying to WindowsCILock November 25, 2025 12:45 — with GitHub Actions Failure

wenju-he temporarily deployed to WindowsCILock November 25, 2025 12:45 — with GitHub Actions Inactive

Maetveis reviewed Nov 25, 2025

View reviewed changes

aelovikov-intel reviewed Nov 25, 2025

View reviewed changes

wenju-he added 2 commits November 26, 2025 00:15

remove -DRESOURCE_DIR per review comment

1f1379a

cache LibspirvDir in SYCLToolchain, use getClangXXExe()

fee3a1a

wenju-he had a problem deploying to WindowsCILock November 26, 2025 00:25 — with GitHub Actions Error

remove if else

d808e0d

wenju-he had a problem deploying to WindowsCILock November 26, 2025 00:28 — with GitHub Actions Failure

wenju-he requested review from Maetveis and aelovikov-intel November 26, 2025 00:35

aelovikov-intel approved these changes Nov 26, 2025

View reviewed changes

wenju-he temporarily deployed to WindowsCILock November 26, 2025 01:08 — with GitHub Actions Inactive

wenju-he had a problem deploying to WindowsCILock November 26, 2025 01:08 — with GitHub Actions Failure

wenju-he temporarily deployed to WindowsCILock November 26, 2025 01:08 — with GitHub Actions Inactive

wenju-he temporarily deployed to WindowsCILock November 26, 2025 01:27 — with GitHub Actions Inactive

wenju-he temporarily deployed to WindowsCILock November 26, 2025 01:56 — with GitHub Actions Inactive

		Driver D{(SYCLToolchain::instance().getPrefix() + "/bin/clang++").str(),
		T.getTriple(), Diags};

	std::string_view getPrefix() const { return Prefix; }
	std::string_view getClangXXExe() const { return ClangXXExe; }

[libclc][Cmake] Change libclc library output library to clang resource dir #20701

Are you sure you want to change the base?

[libclc][Cmake] Change libclc library output library to clang resource dir #20701

Conversation

wenju-he commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Maetveis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

wenju-he commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

aelovikov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wenju-he commented Nov 20, 2025 •

edited

Loading

wenju-he commented Nov 25, 2025 •

edited

Loading