-
Notifications
You must be signed in to change notification settings - Fork 65
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
#0: Use small vector instead of std::vector for shapes to optimize allocations #14281
base: main
Are you sure you want to change the base?
Changes from all commits
f14362e
e35a340
56f17ee
f1cea1e
ccaa821
6ee7987
a669f51
b06ec4d
68195e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ target_link_libraries( | |
magic_enum | ||
fmt | ||
span | ||
Boost::container | ||
) | ||
|
||
target_precompile_headers( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
#include <cstdint> | ||
#include <vector> | ||
#include <optional> | ||
#include <span> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would also allow a lot of the places you currently use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, will do. @patrickroberts I think it's a bit odd that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated all of the usages in ttnn to use |
||
#include "tt_metal/common/constants.hpp" | ||
#include "tt_metal/common/assert.hpp" | ||
#include "tt_metal/third_party/tracy/public/tracy/Tracy.hpp" | ||
|
@@ -25,8 +26,8 @@ enum TensorLayout { | |
template <class T, template <typename...> typename BufferType> | ||
std::vector<T> convert_to_tile_layout( | ||
const BufferType<T>& data, | ||
const std::optional<std::vector<uint32_t>>& tile_shape = std::nullopt, | ||
const std::optional<const std::vector<uint32_t>>& face_shape = std::nullopt) { | ||
std::optional<std::span<const uint32_t>> tile_shape = std::nullopt, | ||
std::optional<std::span<const uint32_t>> face_shape = std::nullopt) { | ||
ZoneScoped; | ||
std::vector<T> result; | ||
result.reserve(data.size()); | ||
|
@@ -79,8 +80,8 @@ std::vector<T> convert_to_tile_layout( | |
template <class T, template <typename...> typename BufferTyp> | ||
std::vector<T> convert_to_flat_layout( | ||
const BufferTyp<T>& data, | ||
const std::optional<std::vector<uint32_t>>& tile_shape = std::nullopt, | ||
const std::optional<const std::vector<uint32_t>>& face_shape = std::nullopt) { | ||
std::optional<std::span<const uint32_t>> tile_shape = std::nullopt, | ||
std::optional<std::span<const uint32_t>> face_shape = std::nullopt) { | ||
ZoneScoped; | ||
std::vector<T> result; | ||
result.reserve(data.size()); | ||
|
@@ -115,7 +116,7 @@ std::vector<T> convert_to_flat_layout( | |
|
||
// Converts a 32-swizzled tilized row-major tensor to a linear 32-zero-padded row-major tensor | ||
template <typename T, template <typename...> typename BufferType> | ||
inline std::vector<T> untilize_nchw(const BufferType<T>& in, const std::vector<std::uint32_t>& shape, const std::optional<std::vector<uint32_t>>& tile_shape = std::nullopt) { | ||
inline std::vector<T> untilize_nchw(const BufferType<T>& in, std::span<const uint32_t> shape, std::optional<std::span<const uint32_t>> tile_shape = std::nullopt) { | ||
ZoneScoped; | ||
auto tile_H = tile_shape.has_value() ? tile_shape.value()[0] : tt::constants::TILE_HEIGHT; | ||
auto tile_W = tile_shape.has_value() ? tile_shape.value()[1] : tt::constants::TILE_WIDTH; | ||
|
@@ -159,7 +160,7 @@ inline std::uint32_t round_up_to_tile(int val, int tile_val) { return (val + til | |
|
||
// Converts a linear non-zero-padded row-major tensor to zero-padded-32 32-swizzled tilized row-major tensor | ||
template <typename T, template <typename...> typename BufferType> | ||
inline std::vector<T> tilize_nchw(const BufferType<T>& in_rowmajor, const std::vector<std::uint32_t>& shape, const std::optional<std::vector<uint32_t>>& tile_shape = std::nullopt) { | ||
inline std::vector<T> tilize_nchw(const BufferType<T>& in_rowmajor, std::span<const uint32_t> shape, std::optional<std::span<const uint32_t>> tile_shape = std::nullopt) { | ||
ZoneScoped; | ||
int H = shape[shape.size() - 2], W = shape[shape.size() - 1]; | ||
auto batch_size = 1; | ||
|
@@ -221,11 +222,11 @@ struct TensAddr { | |
template <typename T, template <typename...> typename BufferType> | ||
inline std::vector<T> convert_layout( | ||
const BufferType<T>& inp, | ||
const std::vector<uint32_t>& shape, | ||
std::span<const uint32_t> shape, | ||
TensorLayout inL, | ||
TensorLayout outL, | ||
const std::optional<std::vector<uint32_t>>& tile_shape = std::nullopt, | ||
const std::optional<const std::vector<uint32_t>>& face_shape = std::nullopt) { | ||
std::optional<std::span<const uint32_t>> tile_shape = std::nullopt, | ||
std::optional<const std::span<const uint32_t>> face_shape = std::nullopt) { | ||
ZoneScoped; | ||
switch (inL) { | ||
case TILED_SWIZZLED: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would appreciate if you follow the pattern of the interface library I did for
span
, like:Then link against the
small_vector
library instead ofBoost::container
directly, which will reduce friction if we move away from boost in the future, which is still under a lot of discussion right now. Also documents in CMake exactly why we pull in that dependency.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrickroberts I'm quite unsure what this would achieve.
If we add
add_library(small_vector INTERFACE)
withsmall_vector.hpp
specified as a source, it would bring more clarity for what's going on, but unfortunately we can't do this, becausesmall_vector.hpp
depends on other parts of ttnn.On the other hand, if we just create an empty interface library, it would be just misleading, because the code
target_link_libraries(ttnn PUBLIC small_vector)
would actually import wholeBoost::container
. Moreover it would provide us any help with removing boost if we will decide to do so.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What it achieves is that if we swap out the implementation of
SmallVector
for something else besidesBoost::container
, we don't have to guess exactly what depends onBoost::container
for that specific purpose, search all the CMake files that have that dependency, and replace it with something else. There would be only one place in CMake to change in order to move to a different dependency.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roughly the same thing goes about a comment. The correct way to fina all usages of Boost::containter is to do a search by
#include <boost/container
. If I leave a comment that I need Boost::container for small_vector, it's likely that someone will just use one of the containers from boost without updating the comment, making it misguiding.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've already put in the work of implementing a proper wrapper container so that nobody should be directly including
<boost/container
anyway. I don't want to go down the path of defending one bad practice with the possibility of another.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You say "search all the CMake files that have that dependency", but this can't be done. Because we can't do
add_library(small_vector INTERFACE small_vector.hpp)
, becausesmall_vector.hpp
depends on a bunch of stuff in ttnn.And again, even if we could do it, wouldn't help in case we decide to replace boost. For example
free_list.cpp
actually depends onBoost::smart_ptr
which isn't shown anywhere in cmake.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hiding boost behind an interface specifying the clear reason why it is brought up sounds like a great idea.
But it actually does not prevent developers from pulling in other types from boost::containers after that interface library is linked. What starts with a neat intention will quickly start lying to us.
Please don't do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evidently I didn't look closely enough on my first pass. The only change in
tt_metal
that I see is a new usage ofstd::span
.Two questions:
boost::span
, onlystd::span
common/test_tiles.hpp
part of the public API? (It's not clear to me from the directory layout)read: it's not clear to me why the new dependency is (a) Boost and (b) public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boost::small_vector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, actually after a second look I agree. It is only used in ttnn right now.
@sminakov-tt I think it can be removed from metal cmake, wdyt?