[llvm][NFC] Move content of ML subdirectory into Analysis

[mtrofin]

Summary

The initial intent was to organize ML stuff in its own directory, but it turns out that conflicts with llvm component layering policies: it is not a component, because subsequent changes want to rely on other analyses, which would create a cycle; and we don't have a reliable, cross-platform mechanism to compile files in a subdirectory, and fit in the existing LLVM build structure.

This change moves the files into Analysis, and subsequent changes will leverage conditional compilation for those that have optional dependencies.

[phosek]

llvm/cmake/modules/AddLLVM.cmake

390

I don't understand this part of the comment, does it mean that we always want to create OBJECT library even either STATIC or SHARED is specified?

698

I'm not a big fan of the name add_implementation_detail as it doesn't really match the rest of functions. How about add_llvm_internal_library or something along those lines?

[mtrofin]

Feedback

[mtrofin]

llvm/cmake/modules/AddLLVM.cmake

390

This documents the existing behavior. Today, this is what happens:

llvm_add_library. |. add_library parameter

STATIC | STATIC
SHARED | SHARED
STATIC SHARED. | SHARED, called ${name}, and a STATIC also, called ${name}_static
STATIC OBJECT. | STATIC called ${name}, and an OBJECT called obj.${name}. The link interface defined when calling llvm_add_library becomes

dependencies of this OBJECT target. The OBJECT target has no link interface

SHARED OBJECT | same as above, s/STATIC/SHARED
OBJECT | same as STATIC OBJECT.

On this last point, my change is technically a breaking change - but it appears nothing uses it in that particular way, i.e. OBJECT without parameters.

On this, I think we should enforce that at least one of the STATIC|OBJECT|SHARED be passed - wdyt?

698

sgtm!

[phosek]

llvm/cmake/modules/AddLLVM.cmake

390

SGTM, SHARED, STATIC and OBJECT should be three different output modes that could be combined to build multiple different outputs in a single llvm_add_library invocation, but at least one of those has to be specified.

[mtrofin]

Avoid breaking change.

[mtrofin]

llvm/cmake/modules/AddLLVM.cmake

390

Addressed: thinking more of it, to avoid breaking changes (and also avoid some asymmetry[1] in the API), I'd propose we add a separate flag - OBJECT_ONLY.

Checking upfront that the flags make sense - i.e. OBJECT_ONLY should come alone.

WDYT?

[1] Today's API looks at BUILD_SHARED_LIBS to figure out defaults if STATIC or SHARED aren't specified. I can see why that may be a good thing.

[mtrofin]

Is this good to go?

Thanks!

[mtrofin]

Gentle reminder - thank you!

[phosek]

LGTM

[beanz]

I have several concerns about this.

(1) This is likely going to cause issues for the Xcode builds. Xcode does not handle object libraries in a remotely sane way.
(2) How does this work with LLVM development installations where we install the static libraries for use component-by-component? CMake doesn't install object libraries.

[beanz]

It is kinda bad form to merge a change that has unaddressed feedback.

I have significant concerns about this change, and I think this change is being made to workaround incorrect implementation that landed in https://reviews.llvm.org/D80579.

We should discuss the problems with this change before any further work goes in on this.

-Chris

[mtrofin]

In D81447#2093437, @beanz wrote:

Sorry, just pushed - I can revert if needed.

I have several concerns about this.

(1) This is likely going to cause issues for the Xcode builds. Xcode does not handle object libraries in a remotely sane way.

Ack - it also fails on windows, it seems, although I'm not sure if this (http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/16497/steps/stage%201%20cmake/logs/stdio) isn't what it says it is - a stale cache. I'll try the XCode case locally (unless a bot picked it up already)

(2) How does this work with LLVM development installations where we install the static libraries for use component-by-component? CMake doesn't install object libraries.

The use case has the object library be built in its container, and then that container is the item being installed. In other words, the object library shouldn't be consumed by anything else than its container.

[mehdi_amini]

Broke our Linux build as well FYI: https://buildkite.com/mlir/mlir-core/builds/5673#854a799f-b1c2-46b4-bbca-0b1d0a5b487f

[mtrofin]

In D81447#2093555, @beanz wrote:

It is kinda bad form to merge a change that has unaddressed feedback.

Apologies again, I didn't see your feedback after the earlier LGTM, until after I pushed. There seems to be a random delay between Phabricator comments being posted, and emails being sent. I suppose I could refresh more actively the Phabricator page.

I have significant concerns about this change, and I think this change is being made to workaround incorrect implementation that landed in https://reviews.llvm.org/D80579.

We should discuss the problems with this change before any further work goes in on this.

-Chris

[mtrofin]

Alternative

[beanz]

LGTM

llvm/cmake/modules/AddLLVM.cmake

set_property(SOURCE ${resource_file}
                "RC_PRODUCT_VERSION=\"${ARG_VERSION_STRING}\"")
 endfunction(set_windows_version_resource_properties)
 
 # llvm_add_library(name sources...
 #   SHARED;STATIC
 #     STATIC by default w/o BUILD_SHARED_LIBS.
 #     SHARED by default w/  BUILD_SHARED_LIBS.
 #   OBJECT
-#     Also create an OBJECT library target. Default if STATIC && SHARED.
+#     Create an OBJECT library target. 
+#     If STATIC or SHARED are
+#     also specified, create an OBJECT library target named obj.${name} in
+#     addition to those.
+#     If both STATIC && SHARED are specified, OBJECT is implied.
 #   MODULE
 #     Target ${name} might not be created on unsupported platforms.
 #     Check with "if(TARGET ${name})".
 #   DISABLE_LLVM_LINK_LLVM_DYLIB
 #     Do not link this library to libLLVM, even if
 #     LLVM_LINK_LLVM_DYLIB is enabled.
 #   OUTPUT_NAME name
 #     Corresponds to OUTPUT_NAME in target properties.
if(ARG_ADDITIONAL_HEADERS)
     set(ARG_ADDITIONAL_HEADERS ADDITIONAL_HEADERS ${ARG_ADDITIONAL_HEADERS})
   endif()
   if(ARG_OBJLIBS)
     set(ALL_FILES ${ARG_OBJLIBS})
   else()
     llvm_process_sources(ALL_FILES ${ARG_UNPARSED_ARGUMENTS} ${ARG_ADDITIONAL_HEADERS})
   endif()
 
+  set(stand_alone_object_lib FALSE)
+  if(NOT ARG_SHARED AND NOT ARG_STATIC AND ARG_OBJECT)
+    set(stand_alone_object_lib TRUE)
+  endif()  
+
+  set(extra_obj_lib FALSE)
+  if (ARG_SHARED AND ARG_STATIC OR (ARG_OBJECT AND NOT stand_alone_object_lib))
+    set(extra_obj_lib TRUE)
+  endif()
+
   if(ARG_MODULE)
     if(ARG_SHARED OR ARG_STATIC)
       message(WARNING "MODULE with SHARED|STATIC doesn't make sense.")
     endif()
     # Plugins that link against a tool are allowed even when plugins in general are not
     if(NOT LLVM_ENABLE_PLUGINS AND NOT (ARG_PLUGIN_TOOL AND LLVM_EXPORT_SYMBOLS_FOR_PLUGINS))
       message(STATUS "${name} ignored -- Loadable modules not supported on this platform.")
       return()
     endif()
   else()
     if(ARG_PLUGIN_TOOL)
       message(WARNING "PLUGIN_TOOL without MODULE doesn't make sense.")
     endif()
     if(BUILD_SHARED_LIBS AND NOT ARG_STATIC)
       set(ARG_SHARED TRUE)
     endif()
-    if(NOT ARG_SHARED)
+    if(NOT ARG_SHARED AND NOT stand_alone_object_lib)
       set(ARG_STATIC TRUE)
     endif()
   endif()
 
-  # Generate objlib
-  if((ARG_SHARED AND ARG_STATIC) OR ARG_OBJECT)
+  # Generate the extra objlib
+  if(extra_obj_lib)
     # Generate an obj library for both targets.
     set(obj_name "obj.${name}")
     add_library(${obj_name} OBJECT EXCLUDE_FROM_ALL
       ${ALL_FILES}
       )
     llvm_update_compile_flags(${obj_name})
     if(CMAKE_GENERATOR STREQUAL "Xcode")
       set(DUMMY_FILE ${CMAKE_CURRENT_BINARY_DIR}/Dummy.c)
if(ARG_SHARED AND ARG_STATIC)
     set(ARG_STATIC)
   endif()
 
   if(ARG_MODULE)
     add_library(${name} MODULE ${ALL_FILES})
   elseif(ARG_SHARED)
     add_windows_version_resource_file(ALL_FILES ${ALL_FILES})
     add_library(${name} SHARED ${ALL_FILES})
-  else()
+  elseif(ARG_STATIC)
     add_library(${name} STATIC ${ALL_FILES})
+  else()
+    add_library(${name} OBJECT ${ALL_FILES})
   endif()
 
   if(ARG_COMPONENT_LIB)
     set_target_properties(${name} PROPERTIES LLVM_COMPONENT TRUE)
     set_property(GLOBAL APPEND PROPERTY LLVM_COMPONENT_LIBS ${name})
   endif()
 
   if(NOT ARG_NO_INSTALL_RPATH)
else()
     # dependency information for this library as defined by LLVMBuild.
     #
     # It would be nice to verify that we have the dependencies for this library
     # name, but using get_property(... SET) doesn't suffice to determine if a
     # property has been set to an empty value.
     get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIB_DEPS_${name})
   endif()
 
-  if(ARG_STATIC)
-    set(libtype PUBLIC)
-  else()
+  if(ARG_SHARED)
     # We can use PRIVATE since SO knows its dependent libs.
     set(libtype PRIVATE)
+  else()
+    set(libtype PUBLIC)
   endif()
 
   target_link_libraries(${name} ${libtype}
       ${ARG_LINK_LIBS}
       ${lib_deps}
       ${llvm_libs}
       )
 
function(llvm_add_library name)
   if(ARG_STATIC AND LLVM_TOUCH_STATIC_LIBRARIES)
     add_custom_command(TARGET ${name}
       POST_BUILD
       COMMAND touch ${LLVM_LIBRARY_DIR}/${CMAKE_STATIC_LIBRARY_PREFIX}${name}${CMAKE_STATIC_LIBRARY_SUFFIX}
       )
   endif()
 endfunction()
 
+# Add a target which is logically and deployment-wise part of another one
+# (owner), but - perhaps because it has optional build dependencies - may be
+# built separately.
+# The owner consumes it via target_link_libraries (or equivalent syntax).
+#
+# PUBLIC creates an OBJECT library, so linking it in the owner translates to 
+# linking the object files in this target as if they were built by the owner.
+#
+# PRIVATE creates a STATIC library, so linking it would drop objects that are
+# not referenced.
+macro (add_implementation_detail name)
+  cmake_parse_arguments(ARG "PUBLIC;PRIVATE" "" "" ${ARGN})
+  if (ARG_PUBLIC)
+    add_llvm_library(${name} OBJECT DISABLE_LLVM_LINK_LLVM_DYLIB 
+      ${ARG_UNPARSED_ARGUMENTS})
+  else()
+    add_llvm_library(${name} STATIC DISABLE_LLVM_LINK_LLVM_DYLIB 
+      ${ARG_UNPARSED_ARGUMENTS})
+  endif()
+endmacro()
+
 function(add_llvm_install_targets target)
   cmake_parse_arguments(ARG "" "COMPONENT;PREFIX;SYMLINK" "DEPENDS" ${ARGN})
   if(ARG_COMPONENT)
     set(component_option -DCMAKE_INSTALL_COMPONENT="${ARG_COMPONENT}")
   endif()
   if(ARG_PREFIX)
     set(prefix_option -DCMAKE_INSTALL_PREFIX="${ARG_PREFIX}")
   endif()

llvm/lib/Analysis/CMakeLists.txt

add_llvm_component_library(LLVMAnalysis
   VectorUtils.cpp
   VFABIDemangling.cpp
 
   ADDITIONAL_HEADER_DIRS
   ${LLVM_MAIN_INCLUDE_DIR}/llvm/Analysis
 
   DEPENDS
   intrinsics_gen
+
+  LINK_LIBS
+  PRIVATE LLVMMLPolicies
   )

llvm/lib/Analysis/LLVMBuild.txt

 ;===- ./lib/Analysis/LLVMBuild.txt -----------------------------*- Conf -*--===;
 ;
 ; Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 ; See https://llvm.org/LICENSE.txt for license information.
 ; SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 ;
 ;===------------------------------------------------------------------------===;
 ;
 ; This is an LLVMBuild description file for the components in this subdirectory.
 ;
 ; For more information on the LLVMBuild system, please see:
 ;
 ;   http://llvm.org/docs/LLVMBuild.html
 ;
 ;===------------------------------------------------------------------------===;
 
-[common]
-subdirectories = ML
-
 [component_0]
 type = Library
 name = Analysis
 parent = Libraries
 required_libraries = BinaryFormat Core Object ProfileData Support

llvm/lib/Analysis/ML/CMakeLists.txt

-add_llvm_component_library(LLVMMLPolicies
+set(BUILD_SHARED_LIBS OFF)
+
+add_implementation_detail(LLVMMLPolicies PUBLIC
   InlineFeaturesAnalysis.cpp
 
   DEPENDS
   intrinsics_gen
   )

llvm/lib/Analysis/ML/LLVMBuild.txt

-;===- ./lib/Analysis/ML/LLVMBuild.txt --------------------------*- Conf -*--===;
-;
-; Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-; See https://llvm.org/LICENSE.txt for license information.
-; SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-;
-;===------------------------------------------------------------------------===;
-;
-; This is an LLVMBuild description file for the components in this subdirectory.
-;
-; For more information on the LLVMBuild system, please see:
-;
-;   http://llvm.org/docs/LLVMBuild.html
-;
-;===------------------------------------------------------------------------===;
-
-[component_0]
-type = Library
-name = MLPolicies
-parent = Analysis
-required_libraries = Core Support

llvm/lib/Passes/LLVMBuild.txt

 ;   http://llvm.org/docs/LLVMBuild.html
 ;
 ;===------------------------------------------------------------------------===;
 
 [component_0]
 type = Library
 name = Passes
 parent = Libraries
-required_libraries = AggressiveInstCombine Analysis MLPolicies CodeGen Core Coroutines IPO InstCombine Scalar Support Target TransformUtils Vectorize Instrumentation
+required_libraries = AggressiveInstCombine Analysis CodeGen Core Coroutines IPO InstCombine Scalar Support Target TransformUtils Vectorize Instrumentation