mpl: refactor placeChildren#8216
Conversation
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
|
Closes #8143 |
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: João Mai <jmai@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
AcKoucher
left a comment
AcKoucher
left a comment
There was a problem hiding this comment.
I really like that we're finally refactoring this.
src/mpl/src/hier_rtlmp.cpp
Outdated
| // be very hard to generate a valid tiling for the clusters. | ||
| // Here, we may want to try setting the area of all standard-cell clusters to 0. | ||
| // This should be only be used in mixed clusters. | ||
| void HierRTLMP::placeChildren(Cluster* parent, bool minimum_target_util) |
There was a problem hiding this comment.
I think that it would be clearer to have this bool named ignore_std_cell_area.
src/mpl/src/hier_rtlmp.cpp
Outdated
| // be very hard to generate a valid tiling for the clusters. | ||
| // Here, we may want to try setting the area of all standard-cell clusters to 0. | ||
| // This should be only be used in mixed clusters. | ||
| void HierRTLMP::placeChildren(Cluster* parent, bool minimum_target_util) |
There was a problem hiding this comment.
With the new naming and refactoring the comment is not needed anymore.
src/mpl/src/hier_rtlmp.cpp
Outdated
| target_utils.push_back(1.6); | ||
| target_dead_spaces.push_back(0.99999); |
There was a problem hiding this comment.
These values should be constexpr.
Also 1.6 --> 1e6.
There was a problem hiding this comment.
I can see a case for creating a const variable for 1e6 and 0.99999, but not constexpr. In the end, both target_utils and target_dead_spaces aren't constexpr (and they're only used to build the utils list and dead spaces lists too), so I don't feel like there's much to gain.
There was a problem hiding this comment.
Sure. It's more of a stylistic matter. There are usages of constexpr in MPL for magic numbers to make them somewhat more evident.
@maliberty Would you have some opinion?
There was a problem hiding this comment.
Is there some meaningful information about these values we can convey in either comments or a variable name?
src/mpl/src/hier_rtlmp.cpp
Outdated
| debugPrint(logger_, | ||
| MPL, | ||
| "hierarchical_macro_placement", | ||
| 1, | ||
| "SA Summary for cluster {}", | ||
| parent->getName()); | ||
|
|
||
| for (auto i = 0; i < sa_containers.size(); i++) { | ||
| debugPrint(logger_, | ||
| MPL, | ||
| "hierarchical_macro_placement", | ||
| 1, | ||
| "sa_id: {}, target_util: {}, target_dead_space: {}", | ||
| i, | ||
| target_util_list[i], | ||
| target_dead_space_list[i]); | ||
|
|
||
| sa_containers[i]->printResults(); | ||
| } |
There was a problem hiding this comment.
I don't think these are useful at all.
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
Signed-off-by: João Mai <jmai@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@joaomai Secure-CI? |
|
CIs are green. |
4 participants
This PR refactors code from placeChildren and placeChildrenUsingMinimumUtil, focusing on de-duplicating the code.
Sections that used to always write to files were also moved to separate functions and called only if debug is enabled.