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.

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

#8129: Build UMD with CMake #9064

Closed
wants to merge 1 commit into from
Closed

#8129: Build UMD with CMake #9064

wants to merge 1 commit into from

Conversation

joelsmithTT
Copy link
Contributor

No description provided.

@tt-rkim
Copy link
Collaborator

tt-rkim commented Jun 3, 2024

Running post commit CI

CMakeLists.txt Outdated
# Build umd_device
include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/umd_device.cmake)

add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/tt_metal/third_party/umd)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we add UMD via ExternalProject ? This avoids any potential namespace pollution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you help me understand the problem you want to avoid with this?
I often edit code in tt_metal/third_party/umd and want the build to react appropriately in this situation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we make this behaviour opt in with a -D option at cmake first execution time?

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://inhzus.io/posts/2023-12-01-cmake-external-project/#intro

you can test with add_subdirectory, but switch to ExternalProject_Add when you check in

@joelsmithTT
Copy link
Contributor Author

I noticed that Metal no longer uses system-installed boost, but relies on a package manager. The current UMD CMakeLists.txt doesn't bother to find_package(Boost REQUIRED) - the compiler finds what it needs in /usr/include.

It seems like we have a few options here:

  • Let UMD continue using system-installed boost. (Whatever problems Metal is solving by using a package manager do not afflict UMD). I think this is OK, but curious if there's an issue with this approach that I don't see.
  • Configure the Metal CMake environment so that UMD can find_package(Boost [...]) and pick up the same one that Metal is using.
  • Something else?

My preference is for UMD to remain simple to build in a standalone manner.

Any thoughts?

@TT-billteng
Copy link
Collaborator

TT-billteng commented Jun 6, 2024

I noticed that Metal no longer uses system-installed boost, but relies on a package manager. The current UMD CMakeLists.txt doesn't bother to find_package(Boost REQUIRED) - the compiler finds what it needs in /usr/include.

Are you saying that you don't want to add find_package to UMD's CMake?

It seems like we have a few options here:

  • Let UMD continue using system-installed boost. (Whatever problems Metal is solving by using a package manager do not afflict UMD). I think this is OK, but curious if there's an issue with this approach that I don't see.
  • Configure the Metal CMake environment so that UMD can find_package(Boost [...]) and pick up the same one that Metal is using.
  • Something else?

My preference is for UMD to remain simple to build in a standalone manner.

Any thoughts?

You could adapt CPM also in UMD :). This could work if you plan on removing Makefile support.

@joelsmithTT
Copy link
Contributor Author

find_package in UMD CMakeLists.txt is fine, but it will pick up the system-installed boost which is not what Metal wants.

I think there's a simpler way to do this that gives Metal more control over the way UMD is built. Please see #9349

Comment on lines +20 to 24
target_link_libraries(tt_metal PUBLIC device metal_common_libs)
add_dependencies(tt_metal umd_device)

target_link_libraries(tt_metal PUBLIC compiler_flags linker_flags metal_header_directories yaml-cpp $<$<BOOL:$ENV{ENABLE_TRACY}>:TracyClient>) # linker_flags = -rdynamic if tracy enabled
target_link_directories(tt_metal PUBLIC ${CMAKE_BINARY_DIR}/lib) # required so tt_metal can find device library
Copy link
Contributor

Choose a reason for hiding this comment

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

If you link to umd_device instead of device I think you can get rid of this target_link_directories? Not sure if the ExternalAdd_Project affects this?

@joelsmithTT joelsmithTT deleted the joelsmith/umd-cmake branch October 9, 2024 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants