[flang] Refine how Clang dependencies are expressed #58663

[Da-Viper]

Summary

Fixes #58663

[Da-Viper]

Hello There,

This is my first contribution and I want to make sure I am doing it the right way.

This is to make the code reusable and easy to read.

I am going to

• convert the macro add_flang_library to a function as I do not see any reason it is a macro (Please correct me if I am wrong)
• explicitly name the cmake_parse_agruments arguments
• do the same as the above for flang/FrontendTool for other flang directories

Possibly extend this to the lldb part too.

Is this the case ?

Thanks

[awarzynski]

Hello @Da-Viper , thanks for sending this!

Would you be able to upload a diff with more context? See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface. You may find it much easier to use Arcanist for all your interactions with Phabricator ;-)

flang/cmake/modules/AddFlang.cmake

66

Is this macro capable of differentiating between DYLIB and non-DYLIB builds?

flang/lib/FrontendTool/CMakeLists.txt

21

Unrelated change

[Da-Viper]

Changed the name of the flang_add_library to reflect it meaning
apply changes to other files

[Da-Viper]

In D140998#4028291, @awarzynski wrote:

Hello @Da-Viper , thanks for sending this!

Would you be able to upload a diff with more context? See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface. You may find it much easier to use Arcanist for all your interactions with Phabricator ;-)

I think I did it the right way take a look

flang/cmake/modules/AddFlang.cmake

66

It is not, It is only for dynamic libs I would update the name to reflect that

66

Looking at previous codes, if you are linking statically it is only clang-cpp that is included else it is anything that is added to the option CLANG_DYLIBS

the rest is then handled by clang_target_link_libraries see AddClang.cmake#L209

[awarzynski]

Sorry, I was meant to reply earlier!

In D140998#4026425, @Da-Viper wrote:

This is my first contribution and I want to make sure I am doing it the right way.

You are - thanks for contributing :)

This is to make the code reusable and easy to read.

Sounds good! One other goal worth considering - alignment with other sub-projects.

I am going to

• convert the macro add_flang_library to a function as I do not see any reason it is a macro (Please correct me if I am wrong)
• explicitly name the cmake_parse_agruments arguments
• do the same as the above for flang/FrontendTool for other flang directories

+1

Possibly extend this to the lldb part too.

Why not do it for all sub-projects? That would be good for consistency. I don't really contribute to other projects - you may want to ask people on Discourse first: https://discourse.llvm.org/.

These changes makes sense- thank you! From my experience, it's good to thoroughly test any CMake changes locally before merging upstream. What CMake configurations have you tested? Basically, you want to make sure that Flang buildbots (https://lab.llvm.org/buildbot/#/builders <--- type "flang" in search box) remain in good shape.

flang/lib/Frontend/CMakeLists.txt

55

Why CLANG_DYLIBS? In LLDB they would use CLANG_LIBS: https://github.com/llvm/llvm-project/blob/beb997799d823aece097494f156c6d277d26571c/lldb/cmake/modules/AddLLDB.cmake#L42

[Da-Viper]

Updating D140998: [flang] Refine how Clang dependencies are expressed #58663

rename CLANG_DYLIBS to CLANG_LIBS

[Da-Viper]

These changes makes sense- thank you! From my experience, it's good to thoroughly test any CMake changes locally before merging upstream. What CMake configurations have you tested? Basically, you want to make sure that Flang buildbots (https://lab.llvm.org/buildbot/#/builders <--- type "flang" in search box) remain in good shape.

I am testing it with the LLVM_BUILD_LLVM_DYLIB option on and off but it takes more than an hour to build on my system

flang/lib/Frontend/CMakeLists.txt

55

I confused one for the other, reverted it

[awarzynski]

In D140998#4031125, @Da-Viper wrote:

I am testing it with the LLVM_BUILD_LLVM_DYLIB option on and off but it takes more than an hour to build on my system

That's probably the main flag to test, so we can be fairly confident that this is correct. And yes, Flang build times are very long :(

LGTM, but please address my comment in flang/lib/FrontendTool/CMakeLists.txt first. Do you have commit access for llvm-project?

[Da-Viper]

flang/lib/FrontendTool/CMakeLists.txt

21

This is now handled by clang_target_link_libraries function that is called in the add_flang_library function as it does the same thing

flang/cmake/modules/AddFlang.cmake

set_windows_version_resource_properties(${name} ${windows_resource_file}
       PRODUCT_NAME "flang")
   endif()
 endmacro()
 
 macro(add_flang_subdirectory name)
   add_llvm_subdirectory(FLANG TOOL ${name})
 endmacro()
 
-macro(add_flang_library name)
+function(add_flang_library name)
+  set(options SHARED STATIC INSTALL_WITH_TOOLCHAIN)
+  set(multiValueArgs ADDITIONAL_HEADERS CLANG_LIBS)
   cmake_parse_arguments(ARG
-    "SHARED;STATIC;INSTALL_WITH_TOOLCHAIN"
+    "${options}"
     ""
-    "ADDITIONAL_HEADERS"
+    "${multiValueArgs}"
     ${ARGN})
+
   set(srcs)
   if (MSVC_IDE OR XCODE)
     # Add public headers
     file(RELATIVE_PATH lib_path
       ${FLANG_SOURCE_DIR}/lib/
       ${CMAKE_CURRENT_SOURCE_DIR})
     if(NOT lib_path MATCHES "^[.][.]")
       file( GLOB_RECURSE headers
else()
     # so we need to handle it here.
     if (BUILD_SHARED_LIBS AND NOT ARG_STATIC)
       set(LIBTYPE SHARED OBJECT)
     else()
       set(LIBTYPE STATIC OBJECT)
     endif()
     set_property(GLOBAL APPEND PROPERTY FLANG_STATIC_LIBS ${name})
   endif()
 
   llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
+  clang_target_link_libraries(${name} PRIVATE ${ARG_CLANG_LIBS})
+
   if (TARGET ${name})
 
     if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY OR ${name} STREQUAL "libflang"
         OR ARG_INSTALL_WITH_TOOLCHAIN)
       get_target_export_arg(${name} Flang export_to_flangtargets UMBRELLA flang-libraries)
       install(TARGETS ${name}
         COMPONENT ${name}
         ${export_to_flangtargets}
if (TARGET ${name})
     set_property(GLOBAL APPEND PROPERTY FLANG_EXPORTS ${name})
   else()
     # Add empty "phony" target
     add_custom_target(${name})
   endif()
 
   set_target_properties(${name} PROPERTIES FOLDER "Flang libraries")
   set_flang_windows_version_resource_properties(${name})
-endmacro(add_flang_library)
+endfunction(add_flang_library)
 
 macro(add_flang_executable name)
   add_llvm_executable(${name} ${ARGN})
   set_target_properties(${name} PROPERTIES FOLDER "Flang executables")
   set_flang_windows_version_resource_properties(${name})
 endmacro(add_flang_executable)
 
 macro(add_flang_tool name)

flang/lib/Frontend/CMakeLists.txt

add_flang_library(flangFrontend
   Extensions
   IRReader
   Option
   Support
   Target
   TargetParser
   FrontendOpenACC
   FrontendOpenMP
-)
-
-if(CLANG_LINK_CLANG_DYLIB)
-  add_dependencies(flangFrontend clang-cpp)
-else()
-  add_dependencies(flangFrontend clangBasic)
-endif()
 
-clang_target_link_libraries(flangFrontend
-  PRIVATE
+  CLANG_LIBS
   clangBasic
   clangDriver
 )

flang/lib/FrontendTool/CMakeLists.txt

 add_flang_library(flangFrontendTool
   ExecuteCompilerInvocation.cpp
 
   DEPENDS
   # This makes sure that the MLIR dependencies of flangFrontend (which are
   # transitively required here) are generated before this target is build.
   flangFrontend
 
   LINK_LIBS
   flangFrontend
   MLIRPass
 
   LINK_COMPONENTS
   Option
   Support
-)
-
-if(CLANG_LINK_CLANG_DYLIB)
-  add_dependencies(flangFrontend clang-cpp)
-else()
-  add_dependencies(flangFrontendTool clangBasic)
-endif()
 
-clang_target_link_libraries(flangFrontendTool
-  PRIVATE
+  CLANG_LIBS
   clangBasic
   clangDriver
 )