Skip to content

[compiler-rt] Disable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON on AIX. #131200

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.

Sign up for GitHub

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 10 commits into from
Mar 19, 2025
Merged

[compiler-rt] Disable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON on AIX. #131200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
55653ee
[compiler-rt] Disable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON on AIX.
DanielCChen Mar 13, 2025
195d4df
Also check UNIX before checking AIX.
DanielCChen Mar 16, 2025
8eecac5
Revert "Also check UNIX before checking AIX."
DanielCChen Mar 16, 2025
b49165a
Use .
DanielCChen Mar 16, 2025
cb04d4a
Revert "Use ."
DanielCChen Mar 16, 2025
8db15c7
Fix a typo.
DanielCChen Mar 16, 2025
929c397
Undo the previous changes.
DanielCChen Mar 17, 2025
8c70651
Simplify the fix: found to way to override teh cache file.
DanielCChen Mar 17, 2025
b24843b
Remove no-action code to avoid confusion.
DanielCChen Mar 17, 2025
8681f3d
Add the same code to runtimes/CMakeLists.txt. Also add a warning mess…
DanielCChen Mar 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler-rt/cmake/base-config-ix.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
extend_path(default_install_path "${COMPILER_RT_INSTALL_PATH}" lib)
set(COMPILER_RT_INSTALL_LIBRARY_DIR "${default_install_path}" CACHE PATH
"Path where built compiler-rt libraries should be installed.")
else(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
else()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not sure if this really needed to change, but it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reivew!
The "old" code is confusing as the condition in else(condition) is ignored in this case. So remove it to make it easy to read.

set(COMPILER_RT_OUTPUT_LIBRARY_DIR
${COMPILER_RT_OUTPUT_DIR}/lib/${COMPILER_RT_OS_DIR})
extend_path(default_install_path "${COMPILER_RT_INSTALL_PATH}" "lib/${COMPILER_RT_OS_DIR}")
Expand Down
13 changes: 10 additions & 3 deletions llvm/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1190,16 +1190,23 @@ endif()
# Build with _XOPEN_SOURCE on AIX, as stray macros in _ALL_SOURCE mode tend to
# break things. In this case we need to enable the large-file API as well.
if (UNIX AND ${CMAKE_SYSTEM_NAME} MATCHES "AIX")
add_compile_definitions(_XOPEN_SOURCE=700)
add_compile_definitions(_LARGE_FILE_API)
add_compile_options(-pthread)
add_compile_definitions(_XOPEN_SOURCE=700)
add_compile_definitions(_LARGE_FILE_API)
add_compile_options(-pthread)

# Modules should be built with -shared -Wl,-G, so we can use runtime linking
# with plugins.
string(APPEND CMAKE_MODULE_LINKER_FLAGS " -shared -Wl,-G")

# Also set the correct flags for building shared libraries.
string(APPEND CMAKE_SHARED_LINKER_FLAGS " -shared")

# Set LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF as AIX doesn't support it
if (LLVM_ENABLE_PER_TARGET_RUNTIME_DIR)
message(WARNING
"LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON is not supported on AIX. LLVM_ENABLE_PER_TARGET_RUNTIME_DIR is set to OFF.")
set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR OFF CACHE BOOL "" FORCE)
endif()
endif()

# Build with _XOPEN_SOURCE on z/OS.
Expand Down
7 changes: 7 additions & 0 deletions runtimes/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,13 @@ endif()
# This can be used to detect whether we're in the runtimes build.
set(LLVM_RUNTIMES_BUILD ON)

if (LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND UNIX AND ${CMAKE_SYSTEM_NAME} MATCHES "AIX")
# Set LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF as AIX doesn't support it
message(WARNING
"LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON is not supported on AIX. LLVM_ENABLE_PER_TARGET_RUNTIME_DIR is set to OFF.")
set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR OFF CACHE BOOL "" FORCE)
endif()
Comment on lines +226 to +231
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not a big fan of forcing the setting to OFF. I have two questions:

  • Is there a reason why people should fundamentally not use the per-target runtime dir on AIX? For example, we don't use it on Apple platforms either by default, but there's no reason why a user couldn't build their own toolchain and decide to use it.
  • Instead of forcing the setting to OFF, we should just FATAL_ERROR if it has been set to ON and we fundamentally can't do it on AIX. This at least avoids adding additional logic in the CMakeLists.txt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment.
Our packaging tool only uses os_dirname path. The driver also is not currently supporting triple path either on AIX. For sure it can be "fixed", but we decided to use only one path on AIX for both clang and flang.
Instead of FATAL_ERROR, we give a warning so that the build can continue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but the mistake in that case is at the level of the CMake invocation. Whoever configured their CMake invocation passed -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON mistakenly, right? We should tell the user that their invocation is wrong with a FATAL_ERROR, not ignore their setting and continue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We consider it as recoverable mis-config. Because the driver and the cmake are consistent regarding where to search and output the library, users shouldn't be surprised as the compile/linking/execution should be all working. The only thing that may surprise users is that if they search for the library and find out it is not in the lib/${triple} path when they have the option ON. For that, we have the warning message in the build.log to explain why.

My first attempt actually followed what APPLE does: check if (LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE) at a few places to neglect the option. I later adopted @arichardson suggestion to put the check at one central place.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that if there is no way to make it work, a FATAL_ERROR would be better. I also think it would be nice to remove the if (APPLE) special cases but that sounds like a much larger change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we do for Apple is also bad, and we should fix that. In fact we want to make it work on Apple platforms but we haven't gotten to it yet.

The reason for my push back is that we don't want to have configuration logic, especially when it's imperative, in the CMake files. We're working really hard to get away from that and that's why I care so strongly about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make it work as there is not technically impossibility, but our decision is not to support two runtime paths on AIX.
@daltenty Could you please comment on if we can issue FATAL_ERROR instead of force setting it to OFF on AIX?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this argument makes a lot of sense to me as well.

we don't want to have configuration logic, especially when it's imperative, in the CMake files

IIUC it sounds like what we are say is that the CMakeLists shouldn’t be making decisions about how the target is configured. Specifying platform configuration defaults is best left to other mechanisms such as caches file (some of which already handle this option for example)

That being the case, if the user specifies LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON we know walking into an config we know happens to be un-implemented for the target. But they asked for what they asked for, so issuing a fatal error (rather than giving them something else) makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks everyone for the input! I will post another PR to change it to using FATAL_ERROR instead of WARNING


foreach(entry ${runtimes})
get_filename_component(projName ${entry} NAME)

Expand Down