Addressed feedback from PR

tenstorrent · Feb 21, 2025 · 4c30c6f · 4c30c6f
1 parent 88e12e9
commit 4c30c6f
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 46 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -270,7 +270,6 @@ endif()
 # For top level install: cmake --build build --target install  or  make/ninja install -C build
 ############################################################################################################################
 # Install for build artifacts that will upload build/lib
-
 install(
     TARGETS
         tt_metal
@@ -280,15 +279,6 @@ install(
         DESTINATION ${CMAKE_INSTALL_LIBDIR}
         COMPONENT tar
 )
-install(
-    TARGETS
-        ttnncpp
-    ARCHIVE
-        DESTINATION ${CMAKE_INSTALL_LIBDIR}
-    LIBRARY
-        DESTINATION ${CMAKE_INSTALL_LIBDIR}
-        COMPONENT tar
-)
 if(WITH_PYTHON_BINDINGS)
     # Install .so into src files for pybinds implementation
     install(
@@ -309,6 +299,17 @@ if(WITH_PYTHON_BINDINGS)
             COMPONENT
             tt_pybinds
     )
+else()
+    #when we don't build python bindings, we generate a dynamic library ttnncpp
+    install(
+        TARGETS
+            ttnncpp
+        ARCHIVE
+            DESTINATION ${CMAKE_INSTALL_LIBDIR}
+        LIBRARY
+            DESTINATION ${CMAKE_INSTALL_LIBDIR}
+            COMPONENT tar
+    )
 endif()
 
 # FIXME(17578): figure out what bits we actually need to ship and omit the rest

diff --git a/tt-train/sources/ttml/CMakeLists.txt b/tt-train/sources/ttml/CMakeLists.txt
@@ -66,9 +66,9 @@ if(NOT TARGET Metalium::Metal)
         message(FATAL_ERROR "libtt_metal.so not found in ${METALIUM_LIB_PATH}")
     endif()
     if(TTNN_LIBRARY)
-        add_library(TT::NN SHARED IMPORTED)
+        add_library(Metalium::PYBIND SHARED IMPORTED)
         set_target_properties(
-            TT::NN::TTNN
+            Metalium::PYBIND
             PROPERTIES
                 IMPORTED_LOCATION
                     "${TTNN_LIBRARY}"
@@ -101,7 +101,7 @@ target_link_libraries(
         pthread
         atomic
         Metalium::Metal
-        TT::NN::TTNN
+        Metalium::PYBIND
         Python::Python
         fmt::fmt-header-only
         magic_enum::magic_enum

diff --git a/ttnn/CMakeLists.txt b/ttnn/CMakeLists.txt
@@ -14,28 +14,49 @@ endif()
 ##################################################
 # Define a method build_library to define all the options and properties for a library
 ##################################################
-function(
-    build_library
-    LIBRARY_NAME
-    SOURCES
-    LINK_DIRECTORIES
-    INCLUDE_DIRS
-    TARGET_LINK_LIBRARIES
-    DYNAMIC_LIBRARY
-)
+function(build_library LIBRARY_NAME ALIAS)
+    set(options
+        SHARED
+        STATIC
+    )
+    set(oneValueArgs) # No single-value args
+    set(multiValueArgs
+        SOURCES
+        PUBLIC_LINK_DIRECTORIES
+        INCLUDE_DIRECTORIES
+        TARGET_LINK_LIBRARIES
+    ) # All are multi-value
+
+    cmake_parse_arguments(BUILD_LIB "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
+
+    # Determine library type
+    if(BUILD_LIB_SHARED)
+        set(LIBRARY_TYPE "SHARED")
+    elseif(BUILD_LIB_STATIC)
+        set(LIBRARY_TYPE "STATIC")
+    else()
+        message(FATAL_ERROR "You must specify SHARED or STATIC for library type")
+    endif()
+
+    # Assign values
+    set(SOURCES "${BUILD_LIB_SOURCES}")
+    set(TARGET_LINK_LIBRARIES "${BUILD_LIB_TARGET_LINK_LIBRARIES}")
+    set(LINK_DIRECTORIES "${BUILD_LIB_LINK_DIRECTORIES}")
+    set(INCLUDE_DIRS "${BUILD_LIB_INCLUDE_DIRS}")
+
     ##################################################
     # Declare and define the library
     ##################################################
-
     #todo: Ideally declaring the library should be the first thing to do, sadly making it shared requires to add sources
-    string(TOUPPER "${LIBRARY_NAME}" LIBRARY_NAME_ALIAS)
-    if(DYNAMIC_LIBRARY)
-        add_library(${LIBRARY_NAME} SHARED ${SOURCES})
-    else()
-        add_library(${LIBRARY_NAME} STATIC ${SOURCES})
-    endif()
+    # Assign values to variables for clarity
 
-    add_library(TT::NN::${LIBRARY_NAME_ALIAS} ALIAS ${LIBRARY_NAME})
+    add_library(
+        ${LIBRARY_NAME}
+        ${LIBRARY_TYPE}
+        ${SOURCES}
+    )
+
+    add_library(Metalium::${ALIAS} ALIAS ${LIBRARY_NAME})
     target_include_directories(${LIBRARY_NAME} PUBLIC "${INCLUDE_DIRS}")
     target_link_directories(${LIBRARY_NAME} PUBLIC ${LINK_DIRECTORIES})
 
@@ -75,7 +96,7 @@ function(
         "$ORIGIN"
     )
 
-    if(DYNAMIC_LIBRARY)
+    if(BUILD_LIB_SHARED)
         #Make sure library built is _ttnn.so / _ttnnpycpp.so and that it can find all it's linked libraries
         #ttnn breaks if - fvisibility = hidden, so CXX_VISIBILITY_PRESET set to default
         set_target_properties(
@@ -1022,10 +1043,10 @@ list(APPEND TTNN_PUBLIC_INCLUDE_DIRS_PYBIND ${TTNN_PUBLIC_INCLUDE_DIRS})
 # Build the libraries!
 ##################################################
 
-set(BUILD_DYNAMIC_LIB ON)
+set(TTNN_CPP_BUILD_TYPE SHARED)
 if(WITH_PYTHON_BINDINGS)
     #ttnncpp will be static when building python bindings
-    set(BUILD_DYNAMIC_LIB OFF)
+    set(TTNN_CPP_BUILD_TYPE STATIC)
 endif()
 
 #there are two ways to build TTNN
@@ -1037,23 +1058,33 @@ endif()
 # ttnn as well, it should build it first without python bindings and then with them.
 # Most of the use cases will link to ttnn without caring a lot about the backend, but
 # for those who need this second use case, now it is officially supported
+
 build_library(
-    "ttnncpp"                       #LIBRARY_NAME
-    "${SOURCES_CPP}"                #SOURCES
-    "${TTNN_PUBLIC_LINK_DIRS}"      #LINK_DIRECTORIES
-    "${TTNN_PUBLIC_INCLUDE_DIRS}"   #INCLUDE_DIRS
-    "${TARGET_LINK_LIBRARIES_CPP}"  #TARGET_LINK_LIBRARIES
-    ${BUILD_DYNAMIC_LIB}            #DYNAMIC_LIBRARY
-    #if we don't build the python binding, is always dynamic
+    "ttnncpp"                        #LIBRARY_NAME
+    "TTNN"                           #ALIAS
+    ${TTNN_CPP_BUILD_TYPE}
+    SOURCES
+        "${SOURCES_CPP}"
+    PUBLIC_LINK_DIRECTORIES
+        "${TTNN_PUBLIC_LINK_DIRS}"
+    INCLUDE_DIRECTORIES
+        "${TTNN_PUBLIC_INCLUDE_DIRS}"
+    TARGET_LINK_LIBRARIES
+        "${TARGET_LINK_LIBRARIES_CPP}"
 )
 
 if(WITH_PYTHON_BINDINGS)
     build_library(
-        "ttnn"                              #LIBRARY_NAME
-        "${SOURCES_PYBIND}"                 #SOURCES
-        "${TTNN_PUBLIC_LINK_DIRS_PYBIND}"   #LINK_DIRECTORIES
-        "${TTNN_PUBLIC_INCLUDE_DIRS_PYBIND}"#INCLUDE_DIRS
-        "${TARGET_LINK_LIBRARIES_PYBIND}"   #TARGET_LINK_LIBRARIES
-        ON # if is built, is always dynamic #DYNAMIC_LIBRARY
+        "ttnn"                           #LIBRARY_NAME
+        "PYBIND"                         #ALIAS
+        SHARED
+        SOURCES
+            "${SOURCES_PYBIND}"
+        PUBLIC_LINK_DIRECTORIES
+            "${TTNN_PUBLIC_LINK_DIRS_PYBIND}"
+        INCLUDE_DIRECTORIES
+            "${TTNN_PUBLIC_INCLUDE_DIRS_PYBIND}"
+        TARGET_LINK_LIBRARIES
+            "${TARGET_LINK_LIBRARIES_PYBIND}"
     )
 endif()