Skip to content
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

Shared lib for tt-forge #257

Merged
merged 5 commits into from
Aug 7, 2024
Merged

Shared lib for tt-forge #257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d7da85
Shared lib for `tt-forge`
mtopalovicTT Jul 31, 2024
7ee5dfe
Adding SPDX header
topke011 Jul 31, 2024
1592277
Remove files which were added by mistake
topke011 Jul 31, 2024
f7edbcc
Addressing comments
mtopalovicTT Aug 1, 2024
c0bc269
Addressing comments
mtopalovicTT Aug 1, 2024
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
8 changes: 7 additions & 1 deletion lib/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,13 @@ add_subdirectory(Conversion)
add_subdirectory(Dialect)
add_subdirectory(Target)

add_mlir_library(TTMLIR STATIC RegisterAll.cpp
# Shared library will include runtime code
# so we only build it if runtime is enabled
if (TTMLIR_ENABLE_RUNTIME)
add_subdirectory(SharedLib)
endif()

add_mlir_library(TTMLIRStatic STATIC RegisterAll.cpp
LINK_LIBS PUBLIC
MLIR
MLIRTTDialect
Expand Down
48 changes: 48 additions & 0 deletions lib/SharedLib/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# Runtime libs from tt-mlir
set(TTNN_RUNTIME_LIBS TTRuntime TTRuntimeTTNN)

# Dependency libs from tt-metal/ttnn project for ttnn runtime
set(TTNN_LIBS TTMETAL_LIBRARY DEVICE_LIBRARY TTEAGER_LIBRARY TTNN_LIBRARY)

# Libs from tt-mlir project
set(TTMLIR_LIBS
TTNNTargetFlatbuffer
MLIRTTDialect
MLIRTTIRDialect
MLIRTTNNDialect
MLIRTTKernelDialect
TTMLIRTTIRToTTNN
MLIRTTMetalDialect
MLIRTTIRTransforms
MLIRTTNNTransforms
MLIRTTIRAnalysis
MLIRTTNNPipelines
TTMLIRTTNNToEmitC
)

# We supply empty.cpp because CMake does not allow creating a library without sources.
add_library(TTMLIR SHARED empty.cpp)
mtopalovicTT marked this conversation as resolved.
Show resolved Hide resolved

add_dependencies(TTMLIR
${TTMLIR_LIBS}
${TTNN_RUNTIME_LIBS}
${TTNN_LIBS}
)

target_link_libraries(TTMLIR PRIVATE
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be PUBLIC, all of the symbols from TTMLIR_LIBS should be made available to front ends. We can drop the -Wl,--whole-archive below. See docs: https://cmake.org/cmake/help/latest/command/target_link_libraries.html#libraries-for-a-target-and-or-its-dependents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left comment above with screen shot but I will write here again. Adding PUBLIC would work if tt-forge and tt-mlir was built as one single project.

target_link_libraries(TTMLIR PUBLIC SomeStaticLib)
If you have above this implies:

  • Link SomeStaticLib to TTMLIR
  • Link SomeStaticLib to any lib which links against TTMLIR directly (translative property of PUBLIC)

So if I have some third lib inside the project say TTMLIR2 and I link against TTMLIR cmake is going to inline all libs from TTMLIR to TTMLIR2.

All above works for all libs inside cmake project which means it wont work between tt-forge and tt-mlir since they are compiled separately (i.e targets from mlir are not visible to forge)

If you are not convinced ping me offline we can discuss :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nsmithtt Ping on this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't matter, TTMLIR target is a final executable, the static libs linked into it if marked PUBLIC means to export their symbols into the dylib. I think there must be something somewhere else in the cmake tree that's messing with this. Anyway, we can revisit, let's get this in as is :)

LLVM
MLIR

# Forces the inclusion of all symbols in the shared object
# This is necessary because the JIT will not be able to find the symbols otherwise
-Wl,--whole-archive
${TTMLIR_LIBS}
${TTNN_RUNTIME_LIBS}
-Wl,--no-whole-archive
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of passing these linker flags we should change the target_link_libraries, PRIVATE to PUBLIC, which should export the symbols of these static libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this work because tt-mlir is built in separate project and its targets are not visible to tt-forge, hence adding public wouldn't change anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think PUBLIC implies -Wl,--no-whole-archive, would you mind trying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean PUBLIC implies -Wl,--whole-archive?
Btw, I tried PUBLIC initially, thats how I've ended up with whole-archive. Just to confirm I tried it again today and same issue. But I will try again to be triple sure :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I don't want to block the review because we should get this in. But something else must be messed up somewhere in the cmake tree, this is what PUBLIC means so I'm not sure why it would be hiding the symbols.


${TTNN_LIBS}

flatbuffers
)

target_link_directories(TTMLIR PRIVATE ${TTMLIR_TOOLCHAIN_DIR}/lib)
3 changes: 3 additions & 0 deletions lib/SharedLib/empty.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
// SPDX-FileCopyrightText: (c) 2024 Tenstorrent AI ULC
//
// SPDX-License-Identifier: Apache-2.0
2 changes: 1 addition & 1 deletion test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,4 @@ add_lit_testsuite(check-ttmlir "Running the ttmlir regression tests"
)
set_target_properties(check-ttmlir PROPERTIES FOLDER "Tests")

add_lit_testsuites(TTMLIR ${CMAKE_CURRENT_SOURCE_DIR} DEPENDS ${TTMLIR_TEST_DEPENDS})
add_lit_testsuites(TTMLIRStatic ${CMAKE_CURRENT_SOURCE_DIR} DEPENDS ${TTMLIR_TEST_DEPENDS})
9 changes: 5 additions & 4 deletions third_party/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,13 @@ set(TTMETAL_LIBRARY_DIR ${PROJECT_SOURCE_DIR}/third_party/tt-metal/src/tt-metal-
set(TTNN_LIBRARY_PATH ${TTMETAL_LIBRARY_DIR}/_ttnn.so)
set(TTMETAL_LIBRARY_PATH ${TTMETAL_LIBRARY_DIR}/libtt_metal.so)
set(TTEAGER_LIBRARY_PATH ${TTMETAL_LIBRARY_DIR}/libtt_eager.so)
set(DEVICE_LIBRARY_PATH ${TTMETAL_LIBRARY_DIR}/libdevice.so)

set(TTMETAL_LIBRARY_DIR ${TTMETAL_LIBRARY_DIR} PARENT_SCOPE)
set(TTNN_LIBRARY_PATH ${TTNN_LIBRARY_PATH} PARENT_SCOPE)
set(TTMETAL_LIBRARY_PATH ${TTMETAL_LIBRARY_PATH} PARENT_SCOPE)
set(TTEAGER_LIBRARY_PATH ${TTEAGER_LIBRARY_PATH} PARENT_SCOPE)

set(DEVICE_LIBRARY_PATH ${DEVICE_LIBRARY_PATH} PARENT_SCOPE)

ExternalProject_Add(
tt-metal
Expand All @@ -58,13 +59,13 @@ ExternalProject_Add(
GIT_REPOSITORY https://github.com/tenstorrent/tt-metal.git
GIT_TAG v0.49.0
GIT_PROGRESS ON
BUILD_BYPRODUCTS ${TTNN_LIBRARY_PATH} ${TTMETAL_LIBRARY_PATH} ${TTEAGER_LIBRARY_PATH}
BUILD_BYPRODUCTS ${TTNN_LIBRARY_PATH} ${TTMETAL_LIBRARY_PATH} ${TTEAGER_LIBRARY_PATH} ${DEVICE_LIBRARY_PATH}
)

set_target_properties(tt-metal PROPERTIES EXCLUDE_FROM_ALL TRUE)

list(APPEND library_names TTNN_LIBRARY TTEAGER_LIBRARY TTMETAL_LIBRARY)
list(APPEND library_paths ${TTNN_LIBRARY_PATH} ${TTMETAL_LIBRARY_PATH} ${TTEAGER_LIBRARY_PATH})
list(APPEND library_names TTNN_LIBRARY TTMETAL_LIBRARY TTEAGER_LIBRARY DEVICE_LIBRARY)
list(APPEND library_paths ${TTNN_LIBRARY_PATH} ${TTMETAL_LIBRARY_PATH} ${TTEAGER_LIBRARY_PATH} ${DEVICE_LIBRARY_PATH})

foreach(lib_name lib_path IN ZIP_LISTS library_names library_paths)
add_library(${lib_name} SHARED IMPORTED GLOBAL)
Expand Down
2 changes: 1 addition & 1 deletion tools/ttmlir-opt/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
get_property(conversion_libs GLOBAL PROPERTY MLIR_CONVERSION_LIBS)
set(LIBS ${dialect_libs} ${conversion_libs} MLIROptLib MLIRTargetCpp TTMLIR)
set(LIBS ${dialect_libs} ${conversion_libs} MLIROptLib MLIRTargetCpp TTMLIRStatic)
add_llvm_executable(ttmlir-opt ttmlir-opt.cpp)

llvm_update_compile_flags(ttmlir-opt)
Expand Down
Loading