Skip to content

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 <[email protected]>
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