Skip to content

Add support for toolchain compilation with LLVM_LINK_LLVM_DYLIB option #1543

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

Merged
merged 2 commits into from
Aug 22, 2022

Conversation

fineg74
Copy link
Contributor

@fineg74 fineg74 commented Jul 8, 2022

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jul 8, 2022

CLA assistant check
All committers have signed the CLA.

@fineg74
Copy link
Contributor Author

fineg74 commented Aug 15, 2022

The purpose of this PR is to break a circular dependency between GenXIntrinsics and llvm libraries to allow building of llvm with shared library configuration. This PR is a first step for this purpose and the follow up PR in llvm branch will use the results of this PR to complete the configuration change

The failing tests are :

  • Unable to locate package clang-15 for In-tree build & tests / Linux (Release, NoSharedLibs)
  • Segmentation fault for a test in Out-of-tree build & tests / Linux (Release)
  • Unable to locate package clang-15 for In-tree build & tests / Linux (Debug, NoSharedLibs)
  • Segmentation fault for a test in Out-of-tree build & tests / Linux (Debug)
  • Unable to locate package clang-15 for In-tree build & tests / Linux (Release, EnableSharedLibs)
    Since the the requested change deals with build configuration rather than with code and does not introduce any new dependency, it is not related to clang-15 package error and is not related to test segmentation fault.

@AlexeySachkov
Copy link
Contributor

@fineg74, could you please simply merge main into your branch and push the PR again? I'm sure that we made some fixes to the CI and everything should pass now

@fineg74
Copy link
Contributor Author

fineg74 commented Aug 18, 2022

@AlexeySachkov I merged the latest main and pushed into PR. Do I need to resubmit the PR ?

@MrSidims MrSidims requested a review from svenvh August 22, 2022 12:48
@svenvh svenvh merged commit 40fd741 into KhronosGroup:main Aug 22, 2022
@aaronpuchert
Copy link

Paradoxically, our builds with LLVM_LINK_LLVM_DYLIB=ON worked before this change, and this change breaks them. We use LLVM_LINK_LLVM_DYLIB because we don't want the static libraries to be used, in fact we don't even ship them. So the DISABLE_LLVM_LINK_LLVM_DYLIB is one problem. Another is that we want shared libraries, but this hardcodes the library as static.

The change seems like a workaround for an issue in intel/llvm that causes regressions for other downstream users. Looking at intel/llvm#6413, I think the patch should have been applied there, or at least limited to that particular build scenario and not all builds with LLVM_LINK_LLVM_DYLIB. Something like

if(INTEL_LLVM AND LLVM_LINK_LLVM_DYLIB)
  add_llvm_library(LLVMSPIRVLib STATIC DISABLE_LLVM_LINK_LLVM_DYLIB ...)
else()
  add_llvm_library(LLVMSPIRVLib ...)
endif()

MrSidims pushed a commit that referenced this pull request Apr 17, 2023
Move the LLVM components to LINK_COMPONENTS because the DEPENDS list has the same semantics as add_dependencies(). In this
case it doesn't include the LLVM components when calling the linker.

It's almost complete revert of #1543
MrSidims pushed a commit to MrSidims/SPIRV-LLVM-Translator that referenced this pull request Apr 27, 2023
Move the LLVM components to LINK_COMPONENTS because the DEPENDS list has the same semantics as add_dependencies(). In this
case it doesn't include the LLVM components when calling the linker.

It's almost complete revert of KhronosGroup#1543
MrSidims added a commit that referenced this pull request Apr 27, 2023
Move the LLVM components to LINK_COMPONENTS because the DEPENDS list has the same semantics as add_dependencies(). In this
case it doesn't include the LLVM components when calling the linker.

It's almost complete revert of #1543

Co-authored-by: Tulio Magno Quites Machado Filho <tuliom@quites.com.br>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants