-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[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
Member
Author
|
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
Contributor
dpalermo
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.
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_librarywas first added, it was apparently copied over fromadd_clang_library, including its logic to determine the library type. It includes a workaround: IfBUILD_SHARED_LIBSis enabled, it should build all libraries as shared, including those that are explicitly marked asSTATIC1, becauseadd_clang_libraryalways passes at least one ofSTATIC/SHAREDtollvm_add_library, andllvm_add_librarycould 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_LIBSis ignored2.I noticed the two implementations of the same functionality, modified only the
add_clang_library, and copied over the result toadd_flang_library3, 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_SHAREDas set bycmake_parse_argumentscontainTRUE/FALSEinstead of the keywords themselves. I could imagine a utility function akin topythonize_boolthat does this.This simplification adds two more changes:
Object libraries are not explicitly requested anymore.
llvm_add_libraryitself 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_LIBSwas removed. It wasFLANG_STATIC_LIBSbefore to copy&paste error of Avoid object libraries in the VS IDE #93519 3 which not used anywhere. In clang,CLANG_STATIC_LIBSis used forclang-shlibto 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 ↩