fix graph_processor buffer_alloc_id calculus #15426
Conversation
mbezuljTT
requested review from
ayerofieiev-tt,
dmakoviichuk-tt,
cfjchu and
TT-BrianLiu
as code owners
mbezuljTT
requested review from
rfurko-tt,
razorback3,
dongjin-na,
abhullar-tt,
pgkeller,
aliuTT,
tt-aho,
tt-dma,
tt-asaigal and
ubcheema
as code owners
|
turns out as much I wanted to hash there was no way to distinguish two buffers of the same spec allocated and deallocated. I had to add buffer id to the buffer object. @abhullar-tt please take a look @ayerofieiev-tt added test, fixed other issue; there was one map id_to_counter that was used between buffers and tensors; this worked just because tensor ids started from 0, and buffer ids were pointers |
mbezuljTT
force-pushed
the
mbezulj/2411-fix_buffer_alloc_id
branch
from
a01fecc to
3bceb6e
Compare
introduce buffer unique id, as an atomic static counter split id_to_counter to buffer_id_to_counter and tensor_id_to_counter
mbezuljTT
force-pushed
the
mbezulj/2411-fix_buffer_alloc_id
branch
from
3bceb6e to
8027a83
Compare
tt_metal/impl/buffers/buffer.hpp
Outdated
| @@ -300,6 +302,8 @@ class Buffer final { | |||
| std::shared_ptr<const BufferPageMapping> buffer_page_mapping_; | |||
|
|
|||
| std::weak_ptr<Buffer> weak_self; | |||
| size_t unique_id_; | |||
There was a problem hiding this comment.
I really don't like the atomic static counter in the buffer, but I don't have a good explanation why. Just letting you know.
@dmakoviichuk-tt PTAL at this PR too.
FWIW atomics are already used for tensor, operation and device ids 🤷 |
|
Alright, let’s go. People had their time. Before merging please make sure to run CI and link post commit runs. Thank you, Marko! |
Problem description
buffer_alloc_id was assigned per memory address, however this created wrong results when capturing composite op that might be allocating and deallocating entire tensors, so two buffers might have the same address.
new buffer alloc id is provided from buffer unique id
old graph capture
new graph capture
