[CPU] Simplify edge clusters collection procedure #28876
Conversation
EgorDuplensky
force-pushed
the
simplify_memory_reuse_new_3
branch
2 times, most recently
from
240f806 to
8f6c29c
Compare
EgorDuplensky
force-pushed
the
simplify_memory_reuse_new_3
branch
from
8f6c29c to
ece7d35
Compare
|
@maxnick Could you please take a look? |
src/plugins/intel_cpu/src/graph.cpp
Outdated
| * - The first edge in the cluster (base edge) must have the status `NeedAllocation`. | ||
| * - All subsequent edges in the cluster must have the status `NotAllocated`. | ||
| * | ||
| * @param clusters A collection of edges. |
There was a problem hiding this comment.
Please add description for the second remaining parameter.
| * - base edge of a cluster is a "ov::element::string" type of edge | ||
| * - base edge of a cluster is a Constant edge | ||
| * | ||
| * @return a remaining number of clusters to process (left partition) | ||
| */ | ||
| static size_t AllocateStringsAndConstants(EdgeClusters& clusters, const GraphContext::CPtr& context) { | ||
| static size_t AllocateStringsAndConstants(EdgeClusters& clusters, size_t remaining, const GraphContext::CPtr& context) { |
There was a problem hiding this comment.
NIT. May be it does make sense to rename remaining to clusters_to_process within these subroutines? remaining makes sense within SolveMemoryReuse implementation, but at this level clusters_to_process would enhance code readability.
There was a problem hiding this comment.
In this case remaining means remaining to allocate. I mean we are allocating the clusters piece by piece and after each step there are remaining clusters which still need to be allocated.
There was a problem hiding this comment.
It's simply more difficult to quickly figure out what does remaining means, and my proposal is to give it more obvious name.
EgorDuplensky
force-pushed
the
simplify_memory_reuse_new_3
branch
from
ece7d35 to
0f6bfb4
Compare
| * - base edge of a cluster is a "ov::element::string" type of edge | ||
| * - base edge of a cluster is a Constant edge | ||
| * | ||
| * @return a remaining number of clusters to process (left partition) | ||
| */ | ||
| static size_t AllocateStringsAndConstants(EdgeClusters& clusters, const GraphContext::CPtr& context) { | ||
| static size_t AllocateStringsAndConstants(EdgeClusters& clusters, size_t remaining, const GraphContext::CPtr& context) { |
There was a problem hiding this comment.
In this case remaining means remaining to allocate. I mean we are allocating the clusters piece by piece and after each step there are remaining clusters which still need to be allocated.
src/plugins/intel_cpu/src/graph.cpp
Outdated
| * - The first edge in the cluster (base edge) must have the status `NeedAllocation`. | ||
| * - All subsequent edges in the cluster must have the status `NotAllocated`. | ||
| * | ||
| * @param clusters A collection of edges. |
|
@maxnick Could you please take a look. |
| MemoryControl::MemorySolution solution; | ||
| EdgeClusters edgeClusters; | ||
| std::tie(solution, edgeClusters, m_outputNodesMemBlocks) = |
There was a problem hiding this comment.
@maxnick FYI
This change actually fixes regression introduced by the previous PR, because structured binding actually creates a new m_outputNodesMemBlocks variable which shadows the original member variable. So, member variable was not actually updated.
EgorDuplensky
force-pushed
the
simplify_memory_reuse_new_3
branch
from
0f6bfb4 to
99ad44a
Compare
|
@maxnick Could you please take a look. |
By utilizing the fact, that the base edge is always the first edge in the cluster.
This helps to avoid unnecessary looping when updating the referencing edges.
Also allocated dynamic output edges before the regions are created, so there is no need to create those regions and then removed them anyway.