-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[flang] Simplify LIBTYPE logic #98072
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
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To elaborate on the idea of making forwarding of flags easier: function(bool_argument_as_forwardable_flag prefix flag)
if ("${${prefix}_${flag}}")
set("${prefix}_${flag}" "${flag}" PARENT_SCOPE)
else()
set("${prefix}_${flag}" "" PARENT_SCOPE)
endif()
endfunction()
bool_argument_as_forwardable_flag(ARG STATIC)
bool_argument_as_forwardable_flag(ARG SHARED)
llvm_add_library(${name} ${ARG_STATIC} ${ARG_SHARED} [...]) |
dpalermo
approved these changes
Jul 12, 2024
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 have verified this fixes the issue with the llvm-flang build when the cmake flag is specified: -DFLANG_RUNTIME_F128_MATH_LIB=libquadmath
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
When the `add_flang_library` was first added, it was apparently copied over from `add_clang_library`, including its logic to determine the library type. It includes a workaround: If `BUILD_SHARED_LIBS` is enabled, it should build all libraries as shared, including those that are explicitly marked as `STATIC`[^1], because `add_clang_library` always passes at least one of `STATIC`/`SHARED` to `llvm_add_library`, and `llvm_add_library` could not distinguish the two cases. Then, the two implementations diverged. For its runtime libraries, Flang requires some libraries to always be static libraries, so if a library is explicitly marked as `STATIC`, `BUILD_SHARED_LIBS` is ignored[^2]. I noticed the two implementations of the same functionality, modified only the `add_clang_library`, and copied over the result to `add_flang_library`[^3], without noticing that they are slightly different. As a result, Flang runtime libraries would be built as shared libraries with `-DBUILD_SHARED_LIBS=ON`, which may break some build configurations[^4]. This PR fixes the problem and at the same time simplifies the library type algorithm by just passing SHARED/STATIC verbatim to `llvm_add_library`. This is effectively what [^2] should have done instead adding more code to undo the workaround of [^1]. Ideally, one would use ``` llvm_add_library(${name} ${ARG_STATIC} ${ARG_SHARED} [...]) ``` but `ARG_STATIC`/`ARG_SHARED` as set by `cmake_parse_arguments` contain `TRUE`/`FALSE` instead of the keywords themselves. I could imagine a utility function akin to `pythonize_bool` that does this. This simplification adds two more changes: 1. Object libraries are not explicitly requested anymore. `llvm_add_library` itself should determine whether an object library is necessary. As the comment notes, using an object library is not without problems and seem of no use here since it works fine without object library when in `XCODE`/`MSVC_IDE` mode. 2. The property `CLANG_STATIC_LIBS` was removed. It was `FLANG_STATIC_LIBS` before to copy&paste error of llvm#93519 [^3] which not used anywhere. In clang, `CLANG_STATIC_LIBS` is used for `clang-shlib` to include all component libraries in a single large library. There is no equivalent `flang-shlib`. [^1]: dbc2a12 [^2]: 3d2e05d [^3]: llvm#93519 [^4]: llvm#93519 (comment)
Zentrik
added a commit
to Zentrik/llvm-project
that referenced
this pull request
Aug 30, 2024
This reverts commit 424ad16.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the
add_flang_library
was first added, it was apparently copied over fromadd_clang_library
, including its logic to determine the library type. It includes a workaround: IfBUILD_SHARED_LIBS
is enabled, it should build all libraries as shared, including those that are explicitly marked asSTATIC
1, becauseadd_clang_library
always passes at least one ofSTATIC
/SHARED
tollvm_add_library
, andllvm_add_library
could not distinguish the two cases.Then, the two implementations diverged. For its runtime libraries, Flang requires some libraries to always be static libraries, so if a library is explicitly marked as
STATIC
,BUILD_SHARED_LIBS
is ignored2.I noticed the two implementations of the same functionality, modified only the
add_clang_library
, and copied over the result toadd_flang_library
3, without noticing that they are slightly different. As a result, Flang runtime libraries would be built as shared libraries with-DBUILD_SHARED_LIBS=ON
, which may break some build configurations4.This PR fixes the problem and at the same time simplifies the library type algorithm by just passing SHARED/STATIC verbatim to
llvm_add_library
. This is effectively what 2 should have done instead adding more code to undo the workaround of 1. Ideally, one would usebut
ARG_STATIC
/ARG_SHARED
as set bycmake_parse_arguments
containTRUE
/FALSE
instead of the keywords themselves. I could imagine a utility function akin topythonize_bool
that does this.This simplification adds two more changes:
Object libraries are not explicitly requested anymore.
llvm_add_library
itself should determine whether an object library is necessary. As the comment notes, using an object library is not without problems and seem of no use here since it works fine without object library forXCODE
/MSVC_IDE
.The property
CLANG_STATIC_LIBS
was removed. It wasFLANG_STATIC_LIBS
before to copy&paste error of Avoid object libraries in the VS IDE #93519 3 which not used anywhere. In clang,CLANG_STATIC_LIBS
is used forclang-shlib
to include all component libraries in a single large library. There is no equivalentflang-shlib
.Footnotes
dbc2a12 ↩ ↩2
3d2e05d ↩ ↩2
Avoid object libraries in the VS IDE #93519 ↩ ↩2
https://github.com/llvm/llvm-project/pull/93519#issuecomment-2192359002 ↩