Remove usage of atom to pb lookup from packing #2932

AmirhosseinPoolad · 2025-03-12T23:10:00Z

Adds new AtomPBLookUp class that holds the atom to/from pb lookup data in the cluster legalizer class. The packing stage now uses that instead of the global context. At the end of packing, AtomPBLookUp gets copied to the global context and everything goes on as normal.

To keep this invariant from being changed in the future, a lock was added to prevent any access to the atom pb data.

Adds new AtomPBLookUp class that holds the atom to/from lookup data in the cluster legalizer class. The packing stage now uses that instead of the global context. At the end of packing, AtomPBLookUp gets copied to the global context and everything goes on as normal.

This lock ensures that the atom_pb_bimap structure in global context is not accessed in packing.

AmirhosseinPoolad · 2025-03-13T20:43:01Z

On a kind of unrelated note: Why are there three different find_pb_graph_pin functions? In TimingGraphBuilder, vpr_utils.cpp and PreClusterDelayCalculator.

AmirhosseinPoolad · 2025-03-13T22:17:00Z

@AlexandreSinger I want to run the nightly tests on this branch before merging, but meanwhile can you take a look at this? I'm done doing the obvious clean ups and fixes.

AlexandreSinger

@AmirhosseinPoolad Thank you so much for this cleanup. It was desperately needed. I think we are getting close to removing this lookup entirely in the packer. I had some comments throughout. Be careful when you rebase onto a very large change like Soheil's formatting change. It can really cause havoc on PRs like this and it will undo Soheil's changes.

I recommend letting Vaughn review this PR as well (when he gets back).

vpr/src/base/atom_lookup.h

vpr/src/pack/atom_pb_bimap.cpp

vpr/src/pack/pack.cpp

AmirhosseinPoolad · 2025-03-17T22:27:03Z

Hi @AlexandreSinger, thanks for the review! I did some more changes based on your comments, do you mind taking another look? Thanks.

AlexandreSinger

Hi @AmirhosseinPoolad

This is coming along great! I agree with your changes so far. I do have some more comments. There are also some comments I made before which I think still need to be addressed.

vpr/src/base/atom_lookup.h

AlexandreSinger · 2025-03-17T22:42:37Z

vpr/src/base/atom_lookup.h

@@ -25,29 +27,49 @@ class AtomLookup {

    typedef vtr::Range<pin_tnode_iterator> pin_tnode_range;

+
+


Nit: extra spaces.

AlexandreSinger · 2025-03-17T22:49:13Z

vpr/src/pack/atom_pb_bimap.cpp

+
+void AtomPBBimap::reset_bimap(const AtomNetlist &netlist) {
+    for (auto blk : netlist.blocks()) {
+        set_atom_pb(blk, nullptr);


Does the atom_to_pb_ bimap not have a clear method? Or do we have to set each element to nullptr to clear it?

It does have one. The reason I didn't use it is at first I thought this was a "just set to nullptr but don't actually erase the data structure" which wouldn't be the same thing as calling clear, but the code for set_atom_pb does actually erase the data structure if one of the arguments is nullptr so it should be equivalent.

AlexandreSinger · 2025-03-17T22:50:05Z

vpr/src/pack/cluster_legalizer.cpp

+     pb->pb_stats->lookahead_input_pins_used = std::vector<std::vector<AtomNetId>>(pb->pb_graph_node->num_input_pin_class);
+     pb->pb_stats->lookahead_output_pins_used = std::vector<std::vector<AtomNetId>>(pb->pb_graph_node->num_output_pin_class);
+ 
+     pb->pb_stats->num_child_blocks_in_pb = 0;


Some style change conflicts that you should probably revert.

AlexandreSinger · 2025-03-17T22:50:20Z

vpr/src/pack/cluster_legalizer.cpp

-                 "\t\t\t Intersect: Atom block %d has no floorplanning constraints\n",
-                 atom_blk_id);
+                "\t\t\t Intersect: Atom block %d has no floorplanning constraints\n",
+                atom_blk_id);


Style changes.

AlexandreSinger · 2025-03-17T22:50:46Z

vpr/src/pack/cluster_legalizer.cpp

-                 "\t\t\t NoC Group: Atom block %d passed cluster, cluster's NoC group was compatible with the atom's group %d\n",
-                 atom_blk_id, (size_t)atom_noc_grp_id);
+                "\t\t\t NoC Group: Atom block %d passed cluster, cluster's NoC group was compatible with the atom's group %d\n",
+                atom_blk_id, (size_t)atom_noc_grp_id);


I am not going to mark every single one. Go through the file and ensure you are not undoing Soheil's changes.

AlexandreSinger · 2025-03-17T22:53:19Z

vpr/src/pack/cluster_legalizer.cpp

    // Copy the options passed by the user
    cluster_legalization_strategy_ = cluster_legalization_strategy;
    enable_pin_feasibility_filter_ = enable_pin_feasibility_filter;
    log_verbosity_ = log_verbosity;
+    atom_pb_lookup_ = AtomPBBimap(g_vpr_ctx.atom().lookup().atom_pb_bimap());


I still think this is the case. Does the atom_pb_bimap have the ability to tell if it is empty or not?

AlexandreSinger · 2025-03-17T22:56:43Z

vpr/src/pack/greedy_candidate_selector.cpp

        if (cluster_legalizer.get_atom_cluster(blk_id) == legalization_cluster_id
-            && is_atom_blk_in_pb(blk_id, atom_ctx.lookup().atom_pb(clustered_blk_id))) {
+            && cluster_legalizer.is_atom_blk_in_cluster_block(blk_id, clustered_blk_id)) {


I am nervous to leave this code in. is_atom_blk_in_cluster_block does not really make sense when it takes two atoms as arguments. I worry this will stay around for a long time. I think this either should be investigated or we keep the "is_atom_blk_in_pb" method.

AlexandreSinger · 2025-03-17T22:58:18Z

vpr/src/pack/pack.cpp

-            g_vpr_ctx.mutable_atom().mutable_lookup().set_atom_clb(blk, ClusterBlockId::INVALID());
-            g_vpr_ctx.mutable_atom().mutable_lookup().set_atom_pb(blk, nullptr);
-        }
+        cluster_legalizer.mutable_atom_pb_lookup().reset_bimap(g_vpr_ctx.atom().netlist());


The cluster legalizer has a reset method which is called in line 303 below. I recommend putting this in there.

vpr/src/pack/cluster_legalizer.cpp

AmirhosseinPoolad · 2025-03-19T00:27:44Z

Oh btw I tried to remove the is_blom_blk function completely and somehow the tests weren't passing. Not sure why.

AlexandreSinger

LGTM! Very well done on this PR Amir; sorry for all the comments. This is a tricky bit of code.

Please resolve the conflicts and ensure it passes CI and then I think this can be merged.

AmirhosseinPoolad and others added 2 commits March 12, 2025 19:08

Merge branch 'master' into refactor_atom_pb_from_packing

02e4f1e

github-actions bot added VPR VPR FPGA Placement & Routing Tool lang-cpp C/C++ code labels Mar 12, 2025

AmirhosseinPoolad requested review from amin1377 and AlexandreSinger March 12, 2025 23:11

AmirhosseinPoolad added 7 commits March 13, 2025 11:36

Remove unused variables in cluster_legalizer.cpp

00aad1b

Remove code duplication between global context and AtomPBBimap

14cff91

Remove pb_free code duplication

f482530

Merge remote-tracking branch 'origin' into refactor_atom_pb_from_packing

1741f76

Merge remote-tracking branch 'origin' into refactor_atom_pb_from_packing

2b9179a

Remove unused variable from update_connection_gain_values

7f3b274

Add atom_pb_bimap locking

c8fe4e0

This lock ensures that the atom_pb_bimap structure in global context is not accessed in packing.

Add documentation to atom_pb_bimap and general cleanup

6128622

AlexandreSinger requested changes Mar 14, 2025

View reviewed changes

Add documentation and clean up AtomPBBimap

8d801d7

AmirhosseinPoolad force-pushed the refactor_atom_pb_from_packing branch from 954a9e2 to 8d801d7 Compare March 17, 2025 17:18

Add doxygen comments to atom_pb_bimap getter functions

4571d72

AlexandreSinger requested changes Mar 17, 2025

View reviewed changes

AmirhosseinPoolad added 4 commits March 18, 2025 20:07

Fix styling regressions

e2ac829

Add reset_bimap helper method to AtomPBBimap

9e0d48a

Remove copying empty bimap from global context to cluster legalizer

4c61867

Refactor is_atom_blk_in_pb function to get two t_pb* arguments

624f251

AmirhosseinPoolad and others added 2 commits March 18, 2025 20:27

Merge branch 'master' into refactor_atom_pb_from_packing

0e6f62a

Fix minor styling issues

dfb6462

AlexandreSinger approved these changes Mar 20, 2025

View reviewed changes

Merge branch 'master' into refactor_atom_pb_from_packing

f77c3c7

AmirhosseinPoolad force-pushed the refactor_atom_pb_from_packing branch from 8f5404f to f77c3c7 Compare March 20, 2025 17:05

AmirhosseinPoolad merged commit ccb2396 into verilog-to-routing:master Mar 20, 2025
36 checks passed

AmirhosseinPoolad deleted the refactor_atom_pb_from_packing branch May 9, 2025 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove usage of atom to pb lookup from packing #2932

Remove usage of atom to pb lookup from packing #2932

AmirhosseinPoolad commented Mar 12, 2025

AmirhosseinPoolad commented Mar 13, 2025

AmirhosseinPoolad commented Mar 13, 2025

AlexandreSinger left a comment

AmirhosseinPoolad commented Mar 17, 2025

AlexandreSinger left a comment

AlexandreSinger Mar 17, 2025

AlexandreSinger Mar 17, 2025

AmirhosseinPoolad Mar 18, 2025

AlexandreSinger Mar 17, 2025

AlexandreSinger Mar 17, 2025

AlexandreSinger Mar 17, 2025

AlexandreSinger Mar 17, 2025

AlexandreSinger Mar 17, 2025

AlexandreSinger Mar 17, 2025

AlexandreSinger Mar 17, 2025

AmirhosseinPoolad commented Mar 19, 2025

AlexandreSinger left a comment

		@@ -25,29 +27,49 @@ class AtomLookup {

		typedef vtr::Range<pin_tnode_iterator> pin_tnode_range;

Remove usage of atom to pb lookup from packing #2932

Remove usage of atom to pb lookup from packing #2932

Conversation

AmirhosseinPoolad commented Mar 12, 2025

AmirhosseinPoolad commented Mar 13, 2025

AmirhosseinPoolad commented Mar 13, 2025

AlexandreSinger left a comment

Choose a reason for hiding this comment

AmirhosseinPoolad commented Mar 17, 2025

AlexandreSinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmirhosseinPoolad commented Mar 19, 2025

AlexandreSinger left a comment

Choose a reason for hiding this comment