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

Place the public API in a special place with a canonical way of accessing them #322

Merged
merged 6 commits into from
Nov 27, 2024
Merged

Place the public API in a special place with a canonical way of accessing them #322

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
554ff00
Place the public API in a special place with a canonical way of acces…
afuller-TT Nov 20, 2024
c8ae6be
Merge remote-tracking branch 'origin/main' into afuller/clarify-api
afuller-TT Nov 21, 2024
6bdc44f
I don't understand why this would change, but the path is clearly wro…
afuller-TT Nov 21, 2024
1c60c36
Merge remote-tracking branch 'origin/main' into afuller/clarify-api
afuller-TT Nov 22, 2024
2e7407c
Merge remote-tracking branch 'origin/main' into afuller/clarify-api
afuller-TT Nov 27, 2024
c861cc5
Merge branch 'main' into afuller/clarify-api
afuller-TT Nov 27, 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
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ message(STATUS "UMD build type: ${CMAKE_BUILD_TYPE}")

include(dependencies)

add_subdirectory(common)
add_subdirectory(device)
add_subdirectory(src)

Expand Down
13 changes: 13 additions & 0 deletions common/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
add_library(umd_common INTERFACE)
joelsmithTT marked this conversation as resolved.
Show resolved Hide resolved
add_library(${PROJECT_NAME}::Common ALIAS umd_common)

target_sources(
umd_common
INTERFACE
assert.hpp
backtrace.hpp
gtest_initializer.hpp # FIXME: this should be tucked away with the tests
logger.hpp
)

target_include_directories(umd_common INTERFACE .)
2 changes: 1 addition & 1 deletion common/assert.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
#include <sstream>
#include <vector>

#include "common/logger.hpp"
#include "logger.hpp"

namespace tt {
template <typename A, typename B>
Expand Down
2 changes: 1 addition & 1 deletion common/logger.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
#include <pybind11/iostream.h>
#endif

#include "common/backtrace.hpp"
#include "backtrace.hpp"
#include "fmt/color.h"
#include "fmt/core.h"
#include "fmt/ostream.h"
Expand Down
4 changes: 2 additions & 2 deletions common/utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ std::string get_abs_path(std::string path) {
std::filesystem::path current_file_path = std::filesystem::path(__FILE__);
std::filesystem::path umd_root;
if (current_file_path.is_absolute()) {
umd_root = current_file_path.parent_path().parent_path();
umd_root = current_file_path.parent_path().parent_path().parent_path();
afuller-TT marked this conversation as resolved.
Show resolved Hide resolved
} else {
std::filesystem::path umd_root_relative =
std::filesystem::relative(std::filesystem::path(__FILE__).parent_path().parent_path(), "../");
std::filesystem::relative(std::filesystem::path(__FILE__).parent_path().parent_path().parent_path(), "../");
umd_root = std::filesystem::canonical(umd_root_relative);
}
std::filesystem::path abs_path = umd_root / path;
Expand Down
54 changes: 28 additions & 26 deletions device/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,41 +2,41 @@ set(POSITION_INDEPENDENT_CODE ON)

generate_fbs_header(${PROJECT_SOURCE_DIR}/device/simulation/tt_simulation_device.fbs)

set(UMD_DEVICE_SRCS
architecture_implementation.cpp
cpuset_lib.cpp
tlb.cpp
tt_cluster_descriptor.cpp
cluster.cpp
tt_silicon_driver_common.cpp
tt_soc_descriptor.cpp
xy_pair.cpp
simulation/tt_simulation_device.cpp
simulation/tt_simulation_host.cpp
blackhole/blackhole_implementation.cpp
grayskull/grayskull_implementation.cpp
wormhole/wormhole_implementation.cpp
coordinate_manager.cpp
blackhole/blackhole_coordinate_manager.cpp
wormhole/wormhole_coordinate_manager.cpp
pcie/pci_device.cpp
hugepage.cpp
)

add_library(device SHARED ${UMD_DEVICE_SRCS})
add_library(device SHARED)
add_library(${PROJECT_NAME}::device ALIAS device)
add_library(${PROJECT_NAME}_device ALIAS device) # For legacy I guess

target_sources(device PRIVATE ${FBS_GENERATED_HEADER})
target_sources(
device
PRIVATE
architecture_implementation.cpp
blackhole/blackhole_coordinate_manager.cpp
blackhole/blackhole_implementation.cpp
cluster.cpp
coordinate_manager.cpp
cpuset_lib.cpp
grayskull/grayskull_implementation.cpp
hugepage.cpp
pcie/pci_device.cpp
simulation/tt_simulation_device.cpp
simulation/tt_simulation_host.cpp
tlb.cpp
tt_cluster_descriptor.cpp
tt_silicon_driver_common.cpp
tt_soc_descriptor.cpp
wormhole/wormhole_coordinate_manager.cpp
wormhole/wormhole_implementation.cpp
xy_pair.cpp
${FBS_GENERATED_HEADER}
)

target_include_directories(
device
PUBLIC
$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}>
broskoTT marked this conversation as resolved.
Show resolved Hide resolved
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/api>
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}>
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}/device>
PRIVATE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>
)

Expand All @@ -49,6 +49,8 @@ target_link_libraries(
flatbuffers
uv
PRIVATE
umd::Common
umd::Firmware
hwloc
rt
Boost::interprocess
Expand Down
6 changes: 3 additions & 3 deletions device/architecture_implementation.h → .../umd/device/architecture_implementation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@
#include <tuple>
#include <vector>

#include "device/tlb.h"
#include "device/tt_arch_types.h"
#include "device/xy_pair.h"
#include "umd/device/tlb.h"
#include "umd/device/tt_arch_types.h"
#include "umd/device/xy_pair.h"

struct tt_driver_host_address_params;
struct tt_driver_eth_interface_params;
Expand Down
4 changes: 2 additions & 2 deletions device/blackhole/blackhole_implementation.h → ...api/umd/device/blackhole_implementation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
#include <array>
#include <stdexcept>

#include "device/architecture_implementation.h"
#include "device/tlb.h"
#include "umd/device/architecture_implementation.h"
#include "umd/device/tlb.h"

namespace tt::umd {

Expand Down
8 changes: 4 additions & 4 deletions device/cluster.h → device/api/umd/device/cluster.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@
#include <unordered_set>
afuller-TT marked this conversation as resolved.
Show resolved Hide resolved
#include <vector>

#include "device/tlb.h"
#include "device/tt_cluster_descriptor_types.h"
#include "device/tt_io.hpp"
#include "fmt/core.h"
#include "pcie/pci_device.hpp"
#include "tt_silicon_driver_common.hpp"
#include "tt_soc_descriptor.h"
#include "tt_xy_pair.h"
#include "umd/device/pci_device.hpp"
#include "umd/device/tlb.h"
#include "umd/device/tt_cluster_descriptor_types.h"
#include "umd/device/tt_io.hpp"

using TLB_DATA = tt::umd::tlb_data;

Expand Down
4 changes: 2 additions & 2 deletions device/coordinate_manager.h → device/api/umd/device/coordinate_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
#include <set>
#include <vector>

#include "device/tt_arch_types.h"
#include "device/tt_xy_pair.h"
#include "umd/device/tt_arch_types.h"
#include "umd/device/tt_xy_pair.h"

class CoordinateManager {
public:
Expand Down
4 changes: 2 additions & 2 deletions device/device_api_metal.h → device/api/umd/device/device_api_metal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@
*/

#pragma once
#include "device/cluster.h"
#include "device/driver_atomics.h"
#include "umd/device/cluster.h"
#include "umd/device/driver_atomics.h"
0 device/driver_atomics.h → device/api/umd/device/driver_atomics.h
View file
Open in desktop
File renamed without changes.
4 changes: 2 additions & 2 deletions device/grayskull/grayskull_implementation.h → ...api/umd/device/grayskull_implementation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@

#include <array>

#include "device/architecture_implementation.h"
#include "device/tlb.h"
#include "architecture_implementation.h"
#include "umd/device/tlb.h"

namespace tt::umd {

Expand Down
2 changes: 1 addition & 1 deletion device/hugepage.h → device/api/umd/device/hugepage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
#include <cstdint>
#include <string>

#include "device/tt_cluster_descriptor_types.h"
#include "umd/device/tt_cluster_descriptor_types.h"

namespace tt::umd {

Expand Down
8 changes: 4 additions & 4 deletions device/pcie/pci_device.hpp → device/api/umd/device/pci_device.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@
#include <unordered_map>
#include <vector>

#include "device/tlb.h"
#include "device/tt_arch_types.h"
#include "device/tt_cluster_descriptor_types.h"
#include "device/tt_xy_pair.h"
#include "umd/device/tlb.h"
#include "umd/device/tt_arch_types.h"
#include "umd/device/tt_cluster_descriptor_types.h"
#include "umd/device/tt_xy_pair.h"

// TODO: this is used up in cluster.cpp but that logic ought to be
// lowered into the PCIDevice class since it is specific to PCIe cards.
Expand Down
0 device/tlb.h → device/api/umd/device/tlb.h
View file
Open in desktop
File renamed without changes.
0 device/tt_arch_types.h → device/api/umd/device/tt_arch_types.h
View file
Open in desktop
File renamed without changes.
6 changes: 3 additions & 3 deletions device/tt_cluster_descriptor.h → ...ce/api/umd/device/tt_cluster_descriptor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@
#include <unordered_set>
#include <vector>

#include "device/tt_cluster_descriptor_types.h"
#include "device/tt_xy_pair.h"
#include "tt_arch_types.h"
#include "umd/device/tt_arch_types.h"
#include "umd/device/tt_cluster_descriptor_types.h"
#include "umd/device/tt_xy_pair.h"

namespace YAML {
class Node;
Expand Down
0 device/tt_cluster_descriptor_types.h → .../umd/device/tt_cluster_descriptor_types.h
View file
Open in desktop
File renamed without changes.
0 device/tt_io.hpp → device/api/umd/device/tt_io.hpp
View file
Open in desktop
File renamed without changes.
0 device/tt_silicon_driver_common.hpp → ...i/umd/device/tt_silicon_driver_common.hpp
View file
Open in desktop
File renamed without changes.
4 changes: 2 additions & 2 deletions device/simulation/tt_simulation_device.h → device/api/umd/device/tt_simulation_device.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
#include <fstream>
#include <vector>

#include "device/cluster.h"
#include "device/simulation/tt_simulation_host.hpp"
#include "umd/device/cluster.h"
#include "umd/device/tt_simulation_host.hpp"

class tt_SimulationDevice : public tt_device {
public:
Expand Down
2 changes: 1 addition & 1 deletion device/simulation/tt_simulation_host.hpp → device/api/umd/device/tt_simulation_host.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#include <memory>
#include <vector>

#include "device/tt_xy_pair.h"
#include "umd/device/tt_xy_pair.h"

#define NNG_SOCKET_PREFIX "ipc:///tmp/"

Expand Down
5 changes: 3 additions & 2 deletions device/tt_soc_descriptor.h → device/api/umd/device/tt_soc_descriptor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@
#include <unordered_map>
#include <vector>

#include "device/coordinate_manager.h"
#include "device/tt_arch_types.h"
#include "fmt/core.h"
#include "tt_xy_pair.h"
#include "umd/device/coordinate_manager.h"
#include "umd/device/tt_arch_types.h"
#include "umd/device/tt_xy_pair.h"

namespace YAML {
class Node;
Expand Down
2 changes: 1 addition & 1 deletion device/tt_xy_pair.h → device/api/umd/device/tt_xy_pair.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

#include <regex>

#include "device/xy_pair.h"
#include "umd/device/xy_pair.h"

using tt_xy_pair = tt::umd::xy_pair;
using tt_cxy_pair = tt::umd::cxy_pair;
Expand Down
4 changes: 2 additions & 2 deletions device/wormhole/wormhole_implementation.h → .../api/umd/device/wormhole_implementation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@

#include <array>

#include "device/architecture_implementation.h"
#include "device/tlb.h"
#include "architecture_implementation.h"
#include "umd/device/tlb.h"

namespace tt::umd {

Expand Down
0 device/xy_pair.h → device/api/umd/device/xy_pair.h
View file
Open in desktop
File renamed without changes.
8 changes: 4 additions & 4 deletions device/architecture_implementation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
//
// SPDX-License-Identifier: Apache-2.0

#include "device/architecture_implementation.h"
#include "umd/device/architecture_implementation.h"

#include "device/blackhole/blackhole_implementation.h"
#include "device/grayskull/grayskull_implementation.h"
#include "device/wormhole/wormhole_implementation.h"
#include "umd/device/blackhole_implementation.h"
#include "umd/device/grayskull_implementation.h"
#include "umd/device/wormhole_implementation.h"

namespace tt::umd {

Expand Down
2 changes: 1 addition & 1 deletion device/blackhole/blackhole_coordinate_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

#pragma once

#include "device/coordinate_manager.h"
#include "umd/device/coordinate_manager.h"

class BlackholeCoordinateManager : public CoordinateManager {
public:
Expand Down
8 changes: 4 additions & 4 deletions device/blackhole/blackhole_implementation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
//
// SPDX-License-Identifier: Apache-2.0

#include "blackhole_implementation.h"
#include "umd/device/blackhole_implementation.h"

#include "device/cluster.h"
#include "src/firmware/riscv/blackhole/eth_interface.h"
#include "src/firmware/riscv/blackhole/host_mem_address_map.h"
#include "blackhole/eth_interface.h"
afuller-TT marked this conversation as resolved.
Show resolved Hide resolved
#include "blackhole/host_mem_address_map.h"
#include "umd/device/cluster.h"

constexpr std::uint32_t NOC_ADDR_LOCAL_BITS = 36; // source: noc_parameters.h, common for WH && BH
constexpr std::uint32_t NOC_ADDR_NODE_ID_BITS = 6; // source: noc_parameters.h, common for WH && BH
Expand Down
22 changes: 10 additions & 12 deletions device/cluster.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// SPDX-FileCopyrightText: (c) 2023 Tenstorrent Inc.
//
// SPDX-License-Identifier: Apache-2.0
#include "cluster.h"
#include "umd/device/cluster.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is umd/device not part of the build include path?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because we haven't discussed what internal access should look like. This PR was demonstrating external access and putting all the public headers in a segregated place to distinguish external vs internal. That aspect is more straightforward / objective.

Internal access is more subjective.

On the one hand, accessing the same way as an external consumer makes it obvious when we're using a public API -- does that matter? For some projects maybe that's useful information and for others it's inconsequential.
It also means that moving a file from internal to external doesn't require changing the includes. In a project highly in flux, perhaps that's a useful convenience. But if structing include ordering to indicate proximity, then it'll need to be updated anyway. And in a more well established project hoisting a module externally is very very rare in the grand scheme of things.

On the other hand, accessing like any other internal header separates on a different concern: I'm conveying that this is a header within this project, but I'm not affected nor do I care whether said header is ALSO available externally or not. Not my concern.

I believe @blozano-tt prefers the former. I prefer the latter. But ultimately neither of us own UMD, so it's you and @broskoTT who should make a call re: what you prefer internal for your project. I can help guide on the CMake change for whichever you prefer.


#include <assert.h>
#include <dirent.h>
Expand Down Expand Up @@ -36,15 +36,13 @@
#include <utility>
#include <vector>

#include "common/logger.hpp"
#include "device/architecture_implementation.h"
#include "device/driver_atomics.h"
#include "device/hugepage.h"
#include "device/tlb.h"
#include "device/tt_arch_types.h"
#include "device/tt_cluster_descriptor.h"
#include "tt_arch_types.h"
#include "tt_cluster_descriptor.h"
#include "logger.hpp"
#include "umd/device/architecture_implementation.h"
#include "umd/device/driver_atomics.h"
#include "umd/device/hugepage.h"
#include "umd/device/tlb.h"
#include "umd/device/tt_arch_types.h"
#include "umd/device/tt_cluster_descriptor.h"
#include "yaml-cpp/yaml.h"

using namespace boost::interprocess;
Expand Down Expand Up @@ -119,8 +117,8 @@ const tt_SocDescriptor& tt_device::get_soc_descriptor(chip_id_t chip_id) const {
#include <iomanip>
#include <thread>

#include "tt_silicon_driver_common.hpp"
#include "tt_xy_pair.h"
#include "umd/device/tt_silicon_driver_common.hpp"
#include "umd/device/tt_xy_pair.h"

struct routing_cmd_t {
uint64_t sys_addr;
Expand Down
Loading
Loading