Simplify autoformat facilities in preparation for removal #17896
Conversation
ayerofieiev-tt
requested review from
bbradelTT,
sjameelTT,
vsureshTT,
edwinleeTT,
patrickroberts,
ntarafdar,
dchenTT,
jaykru-tt,
yugi957,
jvegaTT,
llongTT,
nardoTT,
cfjchu,
dmakoviichuk-tt and
TT-BrianLiu
as code owners
| @@ -117,7 +117,7 @@ Tensor AutoFormat::format_input_tensor( | |||
| padded_shape.to_array_4D(), | |||
| tt::tt_metal::Array4D({0, 0, 0, 0}), | |||
| pad_value, | |||
| false, | |||
| false, /* multicore */ | |||
There was a problem hiding this comment.
I think it is better to put it before false: /* multicore */ false, or it looks like is related to the next parameter.
There was a problem hiding this comment.
Should be /*multicore=*/false,
| ttnn::Shape(unpadded_shape), pad_c, pad_n, pad_h, pad_w); | ||
| [](const std::array<uint32_t, 4>& unpadded_shape) -> std::vector<uint32_t> { | ||
| auto result = | ||
| ttnn::operations::experimental::auto_format::AutoFormat::pad_to_tile_shape(ttnn::Shape(unpadded_shape)); |
There was a problem hiding this comment.
I'd create a standalone function. might be usefull in c++ too.
| ttnn::Shape AutoFormat::pad_to_tile_shape(const ttnn::Shape& unpadded_shape) { | ||
| using namespace tt::constants; | ||
| auto rank = unpadded_shape.rank(); | ||
| TT_ASSERT(rank >= 1, "rank of shape to pad to tile shape must be at least 1."); |
There was a problem hiding this comment.
TT_FATAL might be better.
| @@ -100,6 +100,11 @@ Tensor tensor_to_device( | |||
|
|
|||
| Tensor tensor_cpu(const Tensor& input_tensor, bool blocking, QueueId cq_id) { | |||
| ZoneScoped; | |||
There was a problem hiding this comment.
you can move zonescoped down or graph tracekr up.
| auto a_pad_shape = ttnn::operations::experimental::auto_format::AutoFormat::pad_to_tile_shape( | ||
| temp.get_padded_shape(), false, false, true, true); | ||
| auto a_pad_shape = | ||
| ttnn::operations::experimental::auto_format::AutoFormat::pad_to_tile_shape(temp.get_padded_shape()); |
There was a problem hiding this comment.
nit: can we do something like using ttnn::operations::experimental::auto_format::AutoFormat autoformat and avoid all this long line?
| } | ||
|
|
||
| bool AutoFormat::legal_tile_shape(const ttnn::Shape& shape) { | ||
| return (shape[2] % tt::constants::TILE_HEIGHT == 0 && shape[3] % tt::constants::TILE_WIDTH == 0); |
There was a problem hiding this comment.
nit and more a question than an affirmation... seems that shape[2] and shape[3] have some meaning, wouldn't be better to have some constants like shape[HEIGHT] % tt::constants::TILE_HEIGHT?
Ticket
Link to Github Issue
Problem description
Autoformat is a convoluted mechanism which we want to remove.
But because its convoluted its hard to make in one step.
Want to clean it up a bit as I am trying to better understand how it works.
What's changed
Added comments, removed things which clearly should not be needed.
Checklist