Remove per-layer bounding box - #3785
Conversation
github-actions
Bot
added
VPR
|
In SB-based architectures, the per-layer bounding box method increaes CPD, WL, placement time, and route time. |
|
per_layer_bb / 3D OPIN-based connectivity
|
| ///@brief Returns a view over the nets that are not ignored. | ||
| auto non_ignored_nets() const { | ||
| return nets() | std::views::filter([this](NetId net_id) { return !net_is_ignored(net_id); }); | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm 90% sure this is the reason behind the performance regression. the views library, in particular views::filter has terrible performance compared to raw loops. No fundamental reason other than that current compilers are just not good enough yet and they emit awful assembly.
https://lemire.me/blog/2025/10/05/stdranges-may-not-deliver-the-performance-that-you-expect/
There was a problem hiding this comment.
non_ignored_nets() is only called in periodic paths where the cost is computed from scratch, not in a per-swap hot loop. The article's benchmarks uses chained views with lazy evaluation. Here, we have a single filter.
| return static_cast<int>(std::ranges::count_if(clb_nlist.non_ignored_nets(), [this](ClusterNetId net_id) { | ||
| const t_bb& bb = bb_coords_[net_id]; | ||
| return bb.layer_min != bb.layer_max; | ||
| })); |
There was a problem hiding this comment.
I would also be wary if this one, with the same reasoning as above.
|
SA placement time, titan_other with 3 seeds The runtime gain can attribted to the removal of std::function.
|
|
SA placement time, titan_quick_qor, 3 seeds
|
|
@vaughnbetz When you have a moment, could you take a look at this? |
amin1377
left a comment
amin1377
left a comment
There was a problem hiding this comment.
Thanks, Soheil! I just had a bunch of small comments.
One question: I don’t think per-layer bounding boxes were widely used, but if you think people outside our group may rely on them, it might be worth keeping --place_bounding_box_mode. If it’s set to per_layer, we could just warn the user that the option is deprecated.
I think this can be very confusing. I don't understand why we should keep the command line option if the code is removed. @amin1377 I applied your comments. If you don't have further comments, I think this PR can be merged. |
vaughnbetz
left a comment
vaughnbetz
left a comment
There was a problem hiding this comment.
This simplifies the code quite a lot. It looks good to me, and the speedup is nice!
4 participants
This PR removes per-layer bounding boxes that are used to estimated wirelength in
NetCostHandler.