[shortfin] A number of tweaks for dylib builds. (#434)

* When bundling deps, sets visibility and export control options. * Re-export versioned spdlog symbols so that clients share registry, etc. * Forces inline hidden visibility. * Enables IREE default visibility so that the API is re-exported. * Applies a version script on Linux so that all symbols get versioned. * Makes it possible to use IREE tracing macros in binaries that depend on the dylib. * Enables (thin) LTO by default. * Removes the obsolete find_package() method of depending on IREE. It is incompatible with how we have been doing runtime source builds for a long time. Requires an IREE version bump to pick up iree-org/iree#19068 --------- Co-authored-by: Marius Brehler <marius.brehler@amd.com>
nod-ai · Nov 8, 2024 · 0d949de · 0d949de
1 parent 4cfc295
commit 0d949de
Show file tree

Hide file tree

Showing 6 changed files with 164 additions and 44 deletions.
diff --git a/shortfin/CMakeLists.txt b/shortfin/CMakeLists.txt
@@ -39,11 +39,15 @@ if(NOT WIN32)
   set(CMAKE_POSITION_INDEPENDENT_CODE ON)
 endif()
 
+# Pins
+set(SHORTFIN_IREE_GIT_TAG "iree-2.9.0rc20241108")
+
 # build options
 option(SHORTFIN_BUILD_PYTHON_BINDINGS "Builds Python Bindings" OFF)
 option(SHORTFIN_BUILD_TESTS "Builds C++ tests" ON)
 option(SHORTFIN_BUNDLE_DEPS "Download dependencies instead of using system libraries" ON)
 option(SHORTFIN_ENABLE_TRACING "Enable runtime tracing for iree and shortfin" OFF)
+option(SHORTFIN_ENABLE_LTO "Enables LTO if supported" ON)
 
 set(SHORTFIN_IREE_SOURCE_DIR "" CACHE FILEPATH "Path to IREE source")
 
@@ -79,6 +83,21 @@ include(shortfin_library)
 include(CheckCXXCompilerFlag)
 include(FetchContent)
 
+################################################################################
+# Toolchain features
+################################################################################
+
+if(SHORTFIN_ENABLE_LTO)
+  include(CheckIPOSupported)
+  check_ipo_supported(RESULT SHORTFIN_LTO_SUPPORTED OUTPUT SHORTFIN_LTO_ERROR)
+  if(SHORTFIN_LTO_SUPPORTED)
+    message(STATUS "Shortfin LTO Enabled")
+    set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE)
+  else()
+    message(WARNING "Could not enable LTO (not supported): ${SHORTFIN_LTO_ERROR}")
+  endif()
+endif()
+
 # Enabling ASAN. Note that this will work best if building in a completely
 # bundled fashion and with an ASAN rigged CPython. Otherwise, various LD_PRELOAD
 # hacks are needed. This is merely a develope convenience: people are more
@@ -106,7 +125,9 @@ if(SHORTFIN_SYSTEMS_AMDGPU)
 endif()
 message(STATUS "  - Host")
 
-# Dependencies.
+################################################################################
+# Dependencies
+################################################################################
 
 if(SHORTFIN_BUNDLE_DEPS)
   ## fmt
@@ -139,46 +160,61 @@ if(SHORTFIN_BUNDLE_DEPS)
     GIT_TAG        3634f2ded19e0cf38208c8b86cea9e1d7c8e397d # v0.25.0
   )
 
+  # In order to bundle libraries without conflict, we have to tweak settings.
+  shortfin_push_bundled_lib_options()
+  # Enable spdlog shared library options so we can export it.
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DSPDLOG_SHARED_LIB -Dspdlog_EXPORTS")
   FetchContent_MakeAvailable(fmt spdlog xtl xtensor)
+  shortfin_pop_bundled_lib_options()
 else()
   find_package(spdlog)
   find_package(xtensor)
 endif()
 
-## iree runtime
-
-if (SHORTFIN_IREE_SOURCE_DIR OR SHORTFIN_BUNDLE_DEPS)
-  # Set IREE build flags, if we are building from source
-  set(IREE_BUILD_COMPILER OFF)
-  set(IREE_BUILD_TESTS OFF)
-  set(IREE_BUILD_SAMPLES OFF)
-  # Disable missing submodules error because we are only building the runtime.
-  set(IREE_ERROR_ON_MISSING_SUBMODULES OFF)
-  # Only enable local_sync/local_task/hip drivers for now.
-  set(IREE_HAL_DRIVER_DEFAULTS OFF)
-  set(IREE_HAL_DRIVER_LOCAL_SYNC ON)
-  set(IREE_HAL_DRIVER_LOCAL_TASK ON)
-  if(SHORTFIN_SYSTEMS_AMDGPU)
-    set(IREE_HAL_DRIVER_HIP ON)
-  endif()
-  if (SHORTFIN_ENABLE_TRACING)
-    set(IREE_ENABLE_RUNTIME_TRACING ON)
-    # When using shared libraries there are some issues that need to be
-    # explored more on static initialization order. Something is getting
-    # initialized and is emitting tracy events before tracy objects are
-    # initialized. This can point to some shared library overloading allocation
-    # functions and making them emit tracy events, which are further used in
-    # some static allocation. See https://github.com/wolfpld/tracy/issues/196
-    # for a similar issue and discussion. Using the workaround suggested in
-    # that issue for now. Note that this does not happen when using static
-    # libraries.
-    set(TRACY_DELAYED_INIT ON CACHE BOOL "Enable delayed init for tracy")
-  endif()
+################################################################################
+# IREE
+################################################################################
+
+# Set IREE build flags.
+# We currently rely on IREE to have visible symbols in order to re-export
+# its API for further use.
+# TODO: Turn this back on and use explicit visibility control in the IREE
+# runtime and linker scripts.
+set(IREE_VISIBILITY_HIDDEN OFF)
+set(IREE_BUILD_COMPILER OFF)
+set(IREE_BUILD_TESTS OFF)
+set(IREE_BUILD_SAMPLES OFF)
+# Disable missing submodules error because we are only building the runtime.
+set(IREE_ERROR_ON_MISSING_SUBMODULES OFF)
+# Only enable local_sync/local_task/hip drivers for now.
+set(IREE_HAL_DRIVER_DEFAULTS OFF)
+set(IREE_HAL_DRIVER_LOCAL_SYNC ON)
+set(IREE_HAL_DRIVER_LOCAL_TASK ON)
+if(SHORTFIN_SYSTEMS_AMDGPU)
+  set(IREE_HAL_DRIVER_HIP ON)
+endif()
+if (SHORTFIN_ENABLE_TRACING)
+  set(IREE_ENABLE_RUNTIME_TRACING ON)
+  # When using shared libraries there are some issues that need to be
+  # explored more on static initialization order. Something is getting
+  # initialized and is emitting tracy events before tracy objects are
+  # initialized. This can point to some shared library overloading allocation
+  # functions and making them emit tracy events, which are further used in
+  # some static allocation. See https://github.com/wolfpld/tracy/issues/196
+  # for a similar issue and discussion. Using the workaround suggested in
+  # that issue for now. Note that this does not happen when using static
+  # libraries.
+  set(TRACY_DELAYED_INIT ON CACHE BOOL "Enable delayed init for tracy")
 endif()
 
+# In order to bundle libraries without conflict, we have to tweak settings.
+shortfin_push_bundled_lib_options()
 if(SHORTFIN_IREE_SOURCE_DIR)
+  message(STATUS "Using existing IREE sources: ${SHORTFIN_IREE_SOURCE_DIR}")
   add_subdirectory(${SHORTFIN_IREE_SOURCE_DIR} shortfin_iree SYSTEM EXCLUDE_FROM_ALL)
-elseif (SHORTFIN_BUNDLE_DEPS)
+else()
+  message(STATUS "Fetching IREE sources from tag ${SHORTFIN_IREE_GIT_TAG}")
+
   # TODO: We shouldn't have to pull googletest when we are not building tests.
   #       This needs to be fixed with IREE.
   set(IREE_SUBMODULES "third_party/benchmark third_party/cpuinfo third_party/flatcc third_party/hip-build-deps third_party/googletest")
@@ -188,7 +224,7 @@ elseif (SHORTFIN_BUNDLE_DEPS)
   FetchContent_Declare(
     shortfin_iree
     GIT_REPOSITORY https://github.com/iree-org/iree.git
-    GIT_TAG iree-2.9.0rc20241108
+    GIT_TAG "${SHORTFIN_IREE_GIT_TAG}"
     GIT_SUBMODULES ${IREE_SUBMODULES}
     GIT_SHALLOW TRUE
     SYSTEM
@@ -198,12 +234,12 @@ elseif (SHORTFIN_BUNDLE_DEPS)
   if(NOT shortfin_iree_POPULATED)
     FetchContent_MakeAvailable(shortfin_iree)
   endif()
-else()
-  # Try to find iree using find_package
-  find_package(IREERuntime)
 endif()
+shortfin_pop_bundled_lib_options()
 
-# tests
+################################################################################
+# Tests
+################################################################################
 
 if(SHORTFIN_BUILD_TESTS)
   if (NOT SHORTFIN_BUNDLE_DEPS AND NOT SHORTFIN_IREE_SOURCE_DIR)

diff --git a/shortfin/build_tools/cmake/shortfin_library.cmake b/shortfin/build_tools/cmake/shortfin_library.cmake
@@ -20,26 +20,64 @@ function(shortfin_public_library)
   cmake_parse_arguments(
     _RULE
     ""
-    "NAME"
-    "COMPONENTS"
+    "NAME;LINUX_LD_SCRIPT"
+    "SRCS;COMPONENTS;USAGE_DEPS"
     ${ARGN}
   )
+
+  # Get usage requirements from requested USAGE_DEPS and forward them from the
+  # public library. This is because the contents of the libraries stop at the
+  # public library vs propagating to callers. So we must manually control
+  # the combined usage requirements of the aggregate library.
+  set(_usage_include_directories)
+  set(_usage_compile_definitions)
+  foreach(_usage_dep _shortfin_defs ${_RULE_USAGE_DEPS})
+    get_target_property(_value ${_usage_dep} INTERFACE_INCLUDE_DIRECTORIES)
+    if(_value)
+      list(APPEND _usage_include_directories ${_value})
+    endif()
+    get_target_property(_value ${_usage_dep} INTERFACE_COMPILE_DEFINITIONS)
+    if(_value)
+      list(APPEND _usage_compile_definitions ${_value})
+    endif()
+  endforeach()
+
+  # Useful for debugging include/definition issues.
+  # message(STATUS "Public library ${_RULE_NAME}: Includes = ${_usage_include_directories}")
+  # message(STATUS "Public library ${_RULE_NAME}: Definitions = ${_usage_compile_definitions}")
+
   if(SHORTFIN_BUILD_STATIC)
     # Static library.
     shortfin_components_to_static_libs(_STATIC_COMPONENTS ${_RULE_COMPONENTS})
-    add_library("${_RULE_NAME}-static" STATIC)
+    add_library("${_RULE_NAME}-static" STATIC ${_RULE_SRCS})
+    target_compile_definitions("${_RULE_NAME}-static" INTERFACE
+      _SHORTFIN_USING_DYLIB
+      ${_usage_compile_definitions}
+    )
+    target_include_directories("${_RULE_NAME}-static" INTERFACE ${_usage_include_directories})
     target_link_libraries(
-      "${_RULE_NAME}-static" PUBLIC ${_STATIC_COMPONENTS}
+      "${_RULE_NAME}-static"
+      PRIVATE ${_STATIC_COMPONENTS}
     )
   endif()
 
   if(SHORTFIN_BUILD_DYNAMIC)
     # Dylib library.
     shortfin_components_to_dynamic_libs(_DYLIB_COMPONENTS ${_RULE_COMPONENTS})
-    add_library("${_RULE_NAME}" SHARED)
-    target_compile_definitions("${_RULE_NAME}" INTERFACE _SHORTFIN_USING_DYLIB)
+    add_library("${_RULE_NAME}" SHARED ${_RULE_SRCS})
+    target_compile_definitions("${_RULE_NAME}" INTERFACE
+      _SHORTFIN_USING_DYLIB
+      ${_usage_compile_definitions}
+    )
+    target_include_directories("${_RULE_NAME}" INTERFACE ${_usage_include_directories})
+    if(_RULE_LINUX_LD_SCRIPT)
+      target_link_options("${_RULE_NAME}" PRIVATE
+        "$<$<PLATFORM_ID:Linux>:-Wl,--version-script=${_RULE_LINUX_LD_SCRIPT}>"
+      )
+    endif()
     target_link_libraries(
-      "${_RULE_NAME}" PUBLIC ${_DYLIB_COMPONENTS}
+      "${_RULE_NAME}"
+      PRIVATE ${_DYLIB_COMPONENTS}
     )
     set_target_properties("${_RULE_NAME}" PROPERTIES
       VERSION ${PROJECT_VERSION_MAJOR}.${PROJECT_VERSION_MINOR}
@@ -101,7 +139,13 @@ function(shortfin_cc_component)
       VISIBILITY_INLINES_HIDDEN ON
     )
     target_compile_definitions(${_DYLIB_OBJECTS_NAME}
-      PRIVATE _SHORTFIN_BUILDING_DYLIB)
+      PRIVATE
+        _SHORTFIN_BUILDING_DYLIB
+        # Mate up with spdlog export settings since this is part of the
+        # library that is exporting these symbols.
+        SPDLOG_SHARED_LIB
+        spdlog_EXPORTS
+    )
     target_compile_definitions(${_DYLIB_OBJECTS_NAME} PUBLIC ${_RULE_DEFINES})
   endif()
 endfunction()
@@ -140,3 +184,29 @@ function(shortfin_gtest_test)
   )
   gtest_discover_tests(${_RULE_NAME})
 endfunction()
+
+
+# Make changes to the global compile flags and properties before including
+# bundled deps. This configures various options aimed at making the bundled
+# dependencies private.
+# The effect can be undone with shortfin_pop_bundled_lib_options().
+# After this call, additional changes can be made to CMAKE_CXX_FLAGS as desired.
+macro(shortfin_push_bundled_lib_options)
+  set(SHORTFIN_ORIG_CXX_VISIBILITY_PRESET "${CMAKE_CXX_VISIBILITY_PRESET}")
+  set(SHORTFIN_ORIG_C_VISIBILITY_PRESET "${CMAKE_C_VISIBILITY_PRESET}")
+  set(SHORTFIN_ORIG_VISIBILITY_INLINES_HIDDEN "${CMAKE_VISIBILITY_INLINES_HIDDEN}")
+  set(SHORTFIN_ORIG_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
+
+  # Callers get a known state for visibility controls and can make changes from
+  # there.
+  set(CMAKE_C_VISIBILITY_PRESET "default")
+  set(CMAKE_CXX_VISIBILITY_PRESET "default")
+  set(CMAKE_VISIBILITY_INLINES_HIDDEN ON)
+endmacro()
+
+macro(shortfin_pop_bundled_lib_options)
+  set(CMAKE_CXX_VISIBILITY_PRESET ${SHORTFIN_ORIG_CXX_VISIBILITY_PRESET})
+  set(CMAKE_C_VISIBILITY_PRESET ${SHORTFIN_ORIG_C_VISIBILITY_PRESET})
+  set(CMAKE_VISIBILITY_INLINES_HIDDEN ${SHORTFIN_ORIG_VISIBILITY_INLINES_HIDDEN})
+  set(CMAKE_CXX_FLAGS "${SHORTFIN_ORIG_CXX_FLAGS}")
+endmacro()
diff --git a/shortfin/setup.py b/shortfin/setup.py
@@ -207,6 +207,7 @@ def build_cmake_configuration(CMAKE_BUILD_DIR: Path, extra_cmake_args=[]):
     print(f"CMake build dir: {CMAKE_BUILD_DIR}")
     cmake_args = [
         "-GNinja",
+        "-Wno-dev",
         "--log-level=VERBOSE",
         "-DSHORTFIN_BUNDLE_DEPS=ON",
         f"-DCMAKE_BUILD_TYPE={cfg}",
@@ -221,6 +222,7 @@ def build_cmake_configuration(CMAKE_BUILD_DIR: Path, extra_cmake_args=[]):
             cmake_args.append("-DCMAKE_CXX_COMPILER=clang++")
         add_env_cmake_setting(cmake_args, "CMAKE_LINKER_TYPE", default_value="LLD")
 
+    add_env_cmake_setting(cmake_args, "SHORTFIN_ENABLE_LTO", default_value="ON")
     add_env_cmake_setting(cmake_args, "SHORTFIN_IREE_SOURCE_DIR")
     add_env_cmake_setting(cmake_args, "SHORTFIN_ENABLE_ASAN")
 

diff --git a/shortfin/src/CMakeLists.txt b/shortfin/src/CMakeLists.txt
@@ -20,10 +20,17 @@ message(STATUS "Linking optional components '${_SHORTFIN_LIB_OPTIONAL_COMPONENTS
 shortfin_public_library(
   NAME
     shortfin
+  LINUX_LD_SCRIPT ${CMAKE_CURRENT_SOURCE_DIR}/shortfin.ld
   COMPONENTS
     shortfin_array
     shortfin_local
     shortfin_support
     shortfin_systems_factory
     ${_SHORTFIN_LIB_OPTIONAL_COMPONENTS}
+  USAGE_DEPS
+    spdlog::spdlog
+    fmt::fmt
+    xtensor
+    xtl
+    iree_defs
 )
diff --git a/shortfin/src/shortfin.ld b/shortfin/src/shortfin.ld
@@ -0,0 +1,4 @@
+SHORTFIN_API_3 {
+  /* Generally source level annotations are used. Exceptions only here. */
+  global: *;
+};
diff --git a/shortfin/src/shortfin/support/logging.h b/shortfin/src/shortfin/support/logging.h
@@ -7,6 +7,7 @@
 #ifndef SHORTFIN_SUPPORT_LOGGING_H
 #define SHORTFIN_SUPPORT_LOGGING_H
 
+#include "iree/base/tracing.h"
 #include "shortfin/support/api.h"
 #include "spdlog/spdlog.h"