-
Notifications
You must be signed in to change notification settings - Fork 16.3k
[libclc] Initial support for cross-compiling OpenCL libraries #174022
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -38,7 +38,12 @@ set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS | |||||||||
|
|
||||||||||
| set( LIBCLC_MIN_LLVM 3.9.0 ) | ||||||||||
|
|
||||||||||
| set( LIBCLC_TARGETS_TO_BUILD "all" | ||||||||||
| # A runtimes cross-build should only use the requested target. | ||||||||||
| set( LIBCLC_DEFAULT_TARGET "all" ) | ||||||||||
| if( LLVM_RUNTIMES_BUILD AND LLVM_DEFAULT_TARGET_TRIPLE MATCHES "^nvptx|^amdgcn" ) | ||||||||||
| set( LIBCLC_DEFAULT_TARGET ${LLVM_DEFAULT_TARGET_TRIPLE} ) | ||||||||||
| endif() | ||||||||||
| set( LIBCLC_TARGETS_TO_BUILD ${LIBCLC_DEFAULT_TARGET} | ||||||||||
| CACHE STRING "Semicolon-separated list of libclc targets to build, or 'all'." ) | ||||||||||
|
|
||||||||||
| option( ENABLE_RUNTIME_SUBNORMAL "Enable runtime linking of subnormal support." OFF ) | ||||||||||
|
|
@@ -82,7 +87,10 @@ else() | |||||||||
| # In-tree configuration | ||||||||||
| set( LIBCLC_STANDALONE_BUILD FALSE ) | ||||||||||
|
|
||||||||||
| set( LLVM_PACKAGE_VERSION ${LLVM_VERSION} ) | ||||||||||
| if( NOT LLVM_PACKAGE_VERSION ) | ||||||||||
| set( LLVM_PACKAGE_VERSION ${LLVM_VERSION} ) | ||||||||||
| endif() | ||||||||||
| set( PACKAGE_VERSION ${LLVM_PACKAGE_VERSION} ) | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use LLVM_VERSION_MAJOR? LLVM_PACKAGE_VERSION is not set in in-tree build.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Weird, LLVM_PACKAGE_VERSION should be set if LLVM_VERSION is set from what I know. We need
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little confused what
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
sorry, you're right that It is just that our downstream still uses
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, so just setting both for now is the easiest?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yeah. Perhaps this code llvm-project/flang/CMakeLists.txt Lines 335 to 337 in 80b62cb
|
||||||||||
|
|
||||||||||
| # Note that we check this later (for both build types) but we can provide a | ||||||||||
| # more useful error message when built in-tree. We assume that LLVM tools are | ||||||||||
|
|
||||||||||
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.
would it better to pass
-DLIBCLC_TARGETS_TO_BUILD=${LLVM_DEFAULT_TARGET_TRIPLE}as runtime configuration options so that there is not customization here?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.
Yeah, I'm not completely sold on how to handle this. the fundamental difference here is that we are expecting to build a per-target toolchain. The
libclcproject completely breaks normal CMake projects by just building a ton of different architectures through custom commands.We should only be building a single one, that's the expected way these cross-builds work. I think that's something that should be correct by construction. That being said, functionally it's not a major distinction right now because
libclcdodges every bit of normal CMake. Normally this would implicitly put--target=amdgcn-amd-amdhsaon all your compiles and that's how you'd get the target code. Since we're doing it manually here it's more to fit in with the expected usage. I.e. the following builds the runtime for your targetI'm not sure if there's a more elegant solution to this. If I had my way we'd rewrite all of this to use a custom language and do it the normal way with the above mechanism. Until then I'm not sure. This was the easiest way I could think of to do it. I have in the past added cache files for required GPU configs, but since this is basically overriding what the runtimes build above is trying to do I'm not so sure.
TL;DR
DRUNTIMES_amdgcn-amd-amdhsa_LLVM_ENABLE_RUNTIMES=libclcbuilds for a single target only and is the expected behavior.libclcdoesn't get this because we do everything custom.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.
thanks @jhuber6, the direction looks to me.