Placement move primitive refactoring assignment 2 #2653

robluo · 2024-07-18T06:39:32Z

In placement, we use two methods to update the bounding box: the 3D bounding box (also used for 2D architecture) and the per-layer bounding box. When a swap happens, multiple variables are updated, indicated by the ts (temporary swap) prefix. We can put all these variables into a class. Since only a subset is used depending on the bounding box type, you can examine how these variables are updated throughout the file and write methods for this class to update the appropriate ones.

After some discussion, it was decided to not use inheritance or the type eraser due to potential performance degradation and the lack of readability. In this PR, the control flow that decides 3d or 2d placement are wrapped into a class.

inet_cost, proposed_net_cost, and bb_updated_before grouped in a struct. bb_updated_before renamed to bb_update_status.

AlexandreSinger

Hi @robluo , I agree with this change over using inheritance. I left some comments to improve the PR.

AlexandreSinger · 2024-07-18T12:04:49Z

vpr/src/place/net_cost_handler.cpp

+/**
+ * @brief This class is used to hide control flows needed to distinguish 2d and 3d placement
+ */
+class BB2D3DControlFlow {


The classes and structs defined in this file are polluting the namespace. They either need to be put in the anonymous namespace or made static.

Although it may sound crazy, in the future (in a completely separate cpp file) someone may make a TSInfo struct (they just happen to use the same name) and they will get awful linker errors. Whenever a class/struct/function are declared in a cpp file they should be made static / put in the anonymous namespace to prevent this issue.

We can put all the functions in net_cost_handler into a separate namespace. We are also following the same practice for router lookahead utilities. If you think this is a standard practice, we can at least enforce it for the new code being added to VTR.

I think it should be enforced for new code. and It does not have to be a separate namespace; it need only be static / put in the anonymous namespace. This is common practice for very very large code bases where polluting the global space can be a real problem.

For file scope variables, static makes sense. For struct / class definitions: probably put in anonymous namespace if you want to hide them.

AlexandreSinger · 2024-07-18T12:08:55Z

vpr/src/place/net_cost_handler.cpp

+    void set_ts_bb_coord(const ClusterNetId& net_id);
+    void set_ts_edge(const ClusterNetId& net_id);
+};
+static BB2D3DControlFlow bb_2d_3d_control_flow;


I do not like the name of this class. I would use something like "BBManipulator" or something. Ideally you want to write code in a way that is extensible and self explanatory. BB2D3DControlFlow tells me "There is a problem with the control flow between 2D and 3D BB, and I made this class to encapsulate it" (which honestly is what we are doing), but I would recommend changing the name to something that says "This is a class that will manipulate the BB (whether it is 2D or 3D or any D in the future)".

Well put :)

BBUpdater? Or BBandCostUpdater

vpr/src/place/net_cost_handler.cpp

vaughnbetz

Nice work!

vaughnbetz · 2024-07-19T18:44:19Z

vpr/src/place/net_cost_handler.cpp

@@ -88,14 +94,33 @@ static vtr::vector<ClusterNetId, NetUpdateState> bb_updated_before; // [0...clus
 * layer_ts_bb_coord_new are used.


Also mention: layer_sink_pin_count is only used in the layer bounding box case.
Add @brief at the start of this.

vaughnbetz · 2024-07-19T18:46:05Z

vpr/src/place/net_cost_handler.cpp

+/**
+ * @brief This class is used to hide control flows needed to distinguish 2d and 3d placement
+ */
+class BB2D3DControlFlow {


For file scope variables, static makes sense. For struct / class definitions: probably put in anonymous namespace if you want to hide them.

vaughnbetz · 2024-07-19T18:47:53Z

vpr/src/place/net_cost_handler.cpp

+    void set_ts_bb_coord(const ClusterNetId& net_id);
+    void set_ts_edge(const ClusterNetId& net_id);
+};
+static BB2D3DControlFlow bb_2d_3d_control_flow;


BBUpdater? Or BBandCostUpdater

vaughnbetz · 2024-07-19T18:51:15Z

vpr/src/place/net_cost_handler.cpp

+        ts_info.layer_ts_bb_coord_new.resize(num_nets, std::vector<t_2D_bb>(num_layers, t_2D_bb()));
+    }
+
+    /*This initialize the whole matrix to OPEN which is an invalid value*/


typo: intializes

vaughnbetz · 2024-07-19T18:52:06Z

vpr/src/place/net_cost_handler.cpp

+void BB2D3DControlFlow::init(size_t num_nets, bool cube_bb_in) {
+    const int num_layers = g_vpr_ctx.device().grid.get_num_layers();
+
+    cube_bb = cube_bb_in;


Could comment: we'll use either the 3D BB or per layer BB data structures, but not both.

vaughnbetz · 2024-07-19T18:52:31Z

vpr/src/place/net_cost_handler.cpp

+    }
+
+    /*This initialize the whole matrix to OPEN which is an invalid value*/
+    ts_info.ts_layer_sink_pin_count.resize({num_nets, size_t(num_layers)}, OPEN);


I think this one is only used for per layer BB, so it should be moved inside the if statement.

vaughnbetz · 2024-07-19T18:53:06Z

vpr/src/place/net_cost_handler.cpp

+        ts_info.ts_bb_edge_new.resize(num_nets, t_bb());
+        ts_info.ts_bb_coord_new.resize(num_nets, t_bb());
+    } else {
+        VTR_ASSERT_SAFE(!cube_bb);


I don't have a strong opinion, but usually I don't write asserts that check the else is the opposite of the if condition.

vaughnbetz · 2024-07-19T18:54:57Z

vpr/src/place/net_cost_handler.cpp

+    auto& cluster_ctx = g_vpr_ctx.clustering();
+    if(cube_bb)
+        return cluster_ctx.clb_nlist.pin_type(blk_pin) == PinType::DRIVER;
+    else


I think we can do the if part all the time and it is simpler. If so, we can just get rid of the routine.

vaughnbetz · 2024-07-19T18:56:07Z

vpr/src/place/net_cost_handler.cpp


        t_physical_tile_type_ptr blk_type = physical_tile_type(blk);
        int pin_width_offset = blk_type->pin_width_offset[iblk_pin];
        int pin_height_offset = blk_type->pin_height_offset[iblk_pin];
+        bool is_driver = bb_2d_3d_control_flow.is_driver(blk_type, blk_pin);


Could leave the function if you like it, but I think only the if part is needed (no else).

vaughnbetz · 2024-07-19T18:57:27Z

vpr/src/place/net_cost_handler.h

 */
-void update_move_nets(int num_nets_affected,
-                      const bool cube_bb);
+void update_move_nets(int num_nets_affected);


Do we need num_nets_affected? Can be deleted.

Yulang Luo added 4 commits July 13, 2024 15:21

[vpr][place] refactoring variables in net_cost_handler

5102cc8

inet_cost, proposed_net_cost, and bb_updated_before grouped in a struct. bb_updated_before renamed to bb_update_status.

[vpr][place] fixing comments and better variable names

4e44de6

[vpr][placer] fixed comment's grammar

38c6a85

[vpr][placer] creating ts struct and hiding 2d 3d control flow in class

6843921

github-actions bot added VPR VPR FPGA Placement & Routing Tool lang-cpp C/C++ code labels Jul 18, 2024

robluo marked this pull request as draft July 18, 2024 06:45

AlexandreSinger reviewed Jul 18, 2024

View reviewed changes

Base automatically changed from placement_move_primitive to master July 19, 2024 18:03

vaughnbetz requested changes Jul 19, 2024

View reviewed changes

robluo closed this Jul 22, 2024

		@@ -88,14 +94,33 @@ static vtr::vector<ClusterNetId, NetUpdateState> bb_updated_before; // [0...clus
		* layer_ts_bb_coord_new are used.

Placement move primitive refactoring assignment 2 #2653

Placement move primitive refactoring assignment 2 #2653

Uh oh!

Conversation

robluo commented Jul 18, 2024

Uh oh!

AlexandreSinger left a comment

Choose a reason for hiding this comment

Uh oh!

AlexandreSinger Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vaughnbetz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexandreSinger Jul 18, 2024 •

edited

Loading