-
Notifications
You must be signed in to change notification settings - Fork 800
[libclc][Cmake] Change libclc library output library to clang resource dir #20701
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[libclc][Cmake] Change libclc library output library to clang resource dir #20701
Conversation
…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
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Driver and driver test changes LGTM with some nits. I haven't checked CMake changes, that's mostly coming from upstream right?
| // 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too the same as in #20701 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too the same as in #20701 (comment)
done
| Driver D{(SYCLToolchain::instance().getPrefix() + "/bin/clang++").str(), | ||
| T.getTriple(), Diags}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| (SYCLToolchain::instance().getPrefix() + "/lib/" + LibName).str(); | ||
| std::string LibPath; | ||
| if (LibName.find("libspirv") != std::string::npos) { | ||
| SmallString<256> LibraryPath(D.ResourceDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, resource dir getter should go to the SYCLToolchain class, maybe it should be even pre-computed and stored as a data member.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, resource dir getter should go to the
SYCLToolchainclass, maybe it should be even pre-computed and stored as a data member.
done, moved to a data member of SYCLToolchain
| std::string LibPath; | ||
| if (LibName.find("libspirv") != std::string::npos) { | ||
| SmallString<256> LibraryPath(D.ResourceDir); | ||
| sys::path::append(LibraryPath, "lib", "libclc", LibName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we use sys::path::append in this file (probably erroneously), but can we use simple string concatenation for uniformity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we use
sys::path::appendin this file (probably erroneously), but can we use simple string concatenation for uniformity?
done
aelovikov-intel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sycl-jit LGTM, thanks!
|
kindly ping @intel/dpcpp-clang-driver-reviewers for review |
srividya-sundaram
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Driver changes LGTM
|
@intel/llvm-gatekeepers please merge, thanks |
Restore pulldown omissions:
Additional changes: