Changing subtile selection in the try_centroid_placement of initial_placement #2897

haydar-c · 2025-02-14T06:16:46Z

Summary: changed the subtile selection in the initial_placement to prevent falling into random macro placement while we have available subtiles in the selected location.

Description

There was discrepancy in the MCNC benchmark results while reconstructing from flat placement. The initial results can be seen in the "[APPack] Added Max Displacement Metric #2876". The discrepancy was that even though we were able to reconstruct the clusters with zero error, there were displacements in atoms.

I found that the displacement occurs when we fall into try_place_macro_randomly even though we have available subtiles in the desired location. This seems to be caused by trying to place the block into a randomly selected compatible subtile and if it is occupied falling into try_place_macro_randomly. My change suggests to select the subtile in the desired location from the unused compatible subtitles, falling back into try_place_macro_randomly if we do not have unused compatible subtile in the desired location.

An example of the results indicating discrepancy also given for the ALU4 circuit in the tables below before and after the change.

Before change:

iter_num	pack_time(s)	device_util	total_wl	CPD(ns)	percent_cluster_errors	percent_atoms_displaced	average_atom_displacement	max_atom_displacement
0	0.70	0.67	6398	4.92578	-1	-1	-1	-1
1	0.56	0.68	6413	5.06346	0.227723	0.261242	1.406852	12.000000
2	0.62	0.68	6276	5.0679	0.039604	0.061028	0.265525	10.000000
3	0.58	0.68	6256	5.00301	0.000000	0.004283	0.033191	11.000000
4	0.57	0.68	6284	5.12653	0.000000	0.001071	0.002141	2.000000
5	0.59	0.68	6277	5.0527	0.000000	0.002141	0.020343	10.000000
6	0.57	0.68	6345	5.00962	0.000000	0.002141	0.010707	7.000000
7	0.56	0.68	6318	4.96222	0.000000	0.000000	0.000000	0.000000
8	0.55	0.68	6245	5.06431	0.000000	0.001071	0.011777	11.000000
9	0.55	0.68	6311	4.95586	0.000000	0.000000	0.000000	0.000000
10	0.58	0.68	6339	4.96665	0.000000	0.000000	0.000000	0.000000

After change:

iter_num	pack_time(s)	device_util	total_wl	CPD(ns)	percent_cluster_errors	percent_atoms_displaced	average_atom_displacement	max_atom_displacement
0	0.64	0.67	6271	4.93428	-1	-1	-1	-1
1	0.57	0.68	6362	4.96405	0.227723	0.225910	1.207709	13.000000
2	0.57	0.68	6371	5.08332	0.039604	0.130621	0.509636	11.000000
3	0.54	0.68	6491	4.98076	0.000000	0.000000	0.000000	0.000000
4	0.54	0.68	6446	4.98408	0.000000	0.000000	0.000000	0.000000
5	0.54	0.68	6391	4.97744	0.000000	0.000000	0.000000	0.000000
6	0.59	0.68	6357	4.986	0.000000	0.000000	0.000000	0.000000
7	0.55	0.68	6344	5.04787	0.000000	0.000000	0.000000	0.000000
8	0.57	0.68	6282	5.04263	0.000000	0.000000	0.000000	0.000000
9	0.55	0.68	6287	5.05193	0.000000	0.000000	0.000000	0.000000
10	0.54	0.68	6317	4.99501	0.000000	0.000000	0.000000	0.000000

Related Issue

This is not an issue but related to the results mentioned in "[APPack] Added Flat Placement Reconstruction #2870".

Motivation and Context

To investigate the discrepancy between cluster reconstruction error and atom displacement, especially in the MCNC benchmarks.

How Has This Been Tested?

Basic and strong tests.

Types of changes

Bug fix (change which fixes an issue)
New feature (change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation
I have updated the documentation accordingly
I have added tests to cover my changes
All new and existing tests passed

falling into random macro placement while we have available subtiles in the selected location.

AlexandreSinger

@haydar-c Thank you so much for looking into this! This is clearly a problem with the original centroid placement code where it may suggest a sub_tile at a tile-site which is currently taken!

However, I am not sure if this is the root of our problem. The code that you modified should only be tripped if the location given did not have a sub-tile; but the flat placement does, in fact, provide a sub-tile! I think the real problem here is the neighbor_legal_loc is always set to false in our case (when reconstructing). It really should only be set to false if the sub-tile we produce is legal and the sub-tile is taken. I would suggest adding an else statement at line 585 which checks if there is a block at the location, and setting neighbor_legal_loc to true if not.

The reason to do this is that it appears as though our sub-tile suggestion from flat placement is always being ignored, and just being set to random. We just happen to be lucky and get approximately the one we wanted! Your change is still super important and I recommend keeping it!

vpr/src/place/initial_placement.cpp

…odin) and updated QoR

vpr/src/place/initial_placement.cpp

AlexandreSinger

@haydar-c This change looks great! Just some small comments. Also there seems to be some tests failing.

vpr/src/place/initial_placement.cpp

instead of going to random placement, going to neighbour placement if no available subtile found minor optimization on available subtile vector and other style updates updated QoR for vtr_reg_strong(_odin) based on above placement behaviour

AlexandreSinger

This LGTM, can you please update the golden results for the one failing test whenever you get the chance! Thank you for looking into this!

multiclock_mcnc and updated golden results.

…ncy_debugging

AlexandreSinger · 2025-02-20T21:00:42Z

Merged in Master changes. Will merge after CI passes.

Edit: Actually. Once this passes CI, I may run the nightly tests. I am slightly worried that these changes may change QoR.

haydar-c · 2025-02-20T21:37:43Z

It seems like I need to revise the last test again.

AlexandreSinger · 2025-02-20T21:42:32Z

It seems like I need to revise the last test again.

No worries! Make sure you pull the merge I just did!

vaughnbetz

Some simplifications proposed.

vaughnbetz · 2025-02-21T19:02:31Z

vpr/src/place/initial_placement.cpp

@@ -147,6 +147,26 @@ static bool is_loc_legal(const t_pl_loc& loc,
                         const PartitionRegion& pr,
                         t_logical_block_type_ptr block_type);

+/**
+ * @brief  Helper function to choose a subtile in specified location if compatible and available one exits.


the type is compatible and an available one exists.

vaughnbetz · 2025-02-21T19:04:10Z

vpr/src/place/initial_placement.cpp

+ * @brief  Helper function to choose a subtile in specified location if compatible and available one exits.
+ *  
+ *   @param centroid The centroid location at which the subtile will be selected using its x,y, and layer.
+ *   @param block_type Logical block type of the macro head member.  


Logical block type we would like to place here

vaughnbetz · 2025-02-21T19:05:41Z

vpr/src/place/initial_placement.cpp

+ *   @param centroid The centroid location at which the subtile will be selected using its x,y, and layer.
+ *   @param block_type Logical block type of the macro head member.  
+ *   @param block_loc_registry Placement block location information. To be filled with the location
+ *   where pl_macro is placed.


I think you should remove the "To be filled with the location where pl_macro is placed."
I'd just say:
"Information on where other blocks have been placed."

vaughnbetz · 2025-02-21T19:07:38Z

vpr/src/place/initial_placement.cpp

+ *   @param block_type Logical block type of the macro head member.  
+ *   @param block_loc_registry Placement block location information. To be filled with the location
+ *   where pl_macro is placed.
+ *   @param pr The PartitionRegion of the macro head member - represents its floorplanning constraints, is the size of


I'd just say:
The PartitionRegion of the block we are trying to place - represents its floorplanning constraints; it is the size of the whole chip if the block is not constrained.

vaughnbetz · 2025-02-21T19:08:04Z

vpr/src/place/initial_placement.cpp

+ *   where pl_macro is placed.
+ *   @param pr The PartitionRegion of the macro head member - represents its floorplanning constraints, is the size of
+ *   the whole chip if the macro is not constrained.
+ *   @param rng A random number generator to select subtile from available and compatible ones.


(grammatical nit): a subtile from the available and compatible ones

vaughnbetz · 2025-02-21T19:09:26Z

vpr/src/place/initial_placement.cpp

+ *   the whole chip if the macro is not constrained.
+ *   @param rng A random number generator to select subtile from available and compatible ones.
+ *
+ * @return False if location on chip, legal, but no available subtile found. True otherwise. False leads us to


if the location is on the chip and legal but no available subtile is found at that location.

I'd delete the "False leads us to neighbour placement currently"

vaughnbetz · 2025-02-21T19:12:51Z

vpr/src/place/initial_placement.cpp

+        }
+    }
+
+    return true;


A bit strange -- we return true if we find a subtile, or if we don't even try. Could we return true if we found a legal subtile and false otherwise (and change the calling code). It's a more clear interface.

vaughnbetz · 2025-02-21T19:16:54Z

vpr/src/place/initial_placement.cpp

-    bool neighbor_legal_loc = false;
-    if (!is_loc_legal(centroid_loc, pr, block_type)) {
-        neighbor_legal_loc = find_centroid_neighbor(centroid_loc, block_type, false, blk_loc_registry, rng);
+    if (!is_loc_legal(centroid_loc, pr, block_type) || try_neighbour_due_to_subtile) {


I think you can simplify this to if (!found_legal_subtile) // pick whatever variable name makes sense
if you change the find_subtile_in_location routine to return false if it fails to find a valid subtile at the given location for any reason.

It makes that routine more clear, and the logic here more straightforward.

…results.

AlexandreSinger · 2025-02-21T23:38:18Z

I have launched the Nightly Tests for this branch:
https://github.com/verilog-to-routing/vtr-verilog-to-routing/actions/runs/13467086924

vaughnbetz

Almost there! Just one suggested stylistic simplification

vaughnbetz · 2025-02-21T23:40:36Z

vpr/src/place/initial_placement.cpp

    if (!flat_placement_info.valid) {
        // If a flat placement is not provided, use the centroid of connected
        // blocks which have already been placed.
        unplaced_blocks_to_update_their_score = find_centroid_loc(pl_macro, centroid_loc, blk_loc_registry);
+        if(find_subtile_in_location(centroid_loc, block_type, blk_loc_registry, pr, rng)) {


Simplify to found_legal_subtile = (function return

vaughnbetz · 2025-02-21T23:41:16Z

vpr/src/place/initial_placement.cpp

@@ -567,6 +624,9 @@ static bool try_centroid_placement(const t_pl_macro& pl_macro,
        if (!is_loc_on_chip({centroid_loc.x, centroid_loc.y, centroid_loc.layer}) ||
            !is_loc_legal(centroid_loc, pr, block_type)) {
            unplaced_blocks_to_update_their_score = find_centroid_loc(pl_macro, centroid_loc, blk_loc_registry);
+            if(find_subtile_in_location(centroid_loc, block_type, blk_loc_registry, pr, rng)) {


Simplify to found_legal_subtile = (function return value)

vaughnbetz

LGTM! Thanks.

changed the subtile selection in the initial_placement to prevent

e0522b0

falling into random macro placement while we have available subtiles in the selected location.

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

AlexandreSinger requested changes Feb 14, 2025

View reviewed changes

vaughnbetz reviewed Feb 14, 2025

View reviewed changes

vpr/src/place/initial_placement.cpp Outdated Show resolved Hide resolved

haydar-c added 4 commits February 16, 2025 20:56

hoisted the subtile selection to find_centroid_loc

8cc499e

added the selection from available subtiles to hoisted selection

489698f

changed the config (seed) of the strong_sdc tests of vtr_reg_strong(_…

d7d6fc9

…odin) and updated QoR

updated the helper find_subtile_in_location structure and comments.

0dba701

AlexandreSinger reviewed Feb 19, 2025

View reviewed changes

vpr/src/place/initial_placement.cpp Outdated Show resolved Hide resolved

AlexandreSinger reviewed Feb 19, 2025

View reviewed changes

vpr/src/place/initial_placement.cpp Show resolved Hide resolved

AlexandreSinger requested changes Feb 19, 2025

View reviewed changes

vpr/src/place/initial_placement.cpp Outdated Show resolved Hide resolved

vpr/src/place/initial_placement.cpp Show resolved Hide resolved

vpr/src/place/initial_placement.cpp Show resolved Hide resolved

Updates based on the review

463dd1c

instead of going to random placement, going to neighbour placement if no available subtile found minor optimization on available subtile vector and other style updates updated QoR for vtr_reg_strong(_odin) based on above placement behaviour

AlexandreSinger approved these changes Feb 20, 2025

View reviewed changes

haydar-c and others added 2 commits February 20, 2025 15:49

changed the task list of vtr_reg_multiclock_odin for

6664fcb

multiclock_mcnc and updated golden results.

Merge branch 'master' into reconstruction_from_flat_placement_dicrepa…

05e2917

…ncy_debugging

vaughnbetz requested changes Feb 21, 2025

View reviewed changes

simplified find_subtile_in_location, improved commenting and updated …

03b820a

…results.

vaughnbetz requested changes Feb 21, 2025

View reviewed changes

AlexandreSinger mentioned this pull request Feb 22, 2025

[CI] Made Nightly Tests Continue on Error #2905

Closed

haydar-c added 2 commits February 22, 2025 12:55

stylistic simplification

8765153

manual QoR update for vtr_reg_multiclock_odin

b308dac

vaughnbetz approved these changes Feb 23, 2025

View reviewed changes

vaughnbetz merged commit 3664c57 into master Feb 23, 2025
36 checks passed

vaughnbetz deleted the reconstruction_from_flat_placement_dicrepancy_debugging branch February 23, 2025 01:12

AlexandreSinger mentioned this pull request Feb 24, 2025

[CI] Update Golden Results for Nightly Tests 1 and 3 #2907

Closed

Changing subtile selection in the try_centroid_placement of initial_placement #2897

Changing subtile selection in the try_centroid_placement of initial_placement #2897

Uh oh!

Conversation

haydar-c commented Feb 14, 2025 • edited by ueqri Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

AlexandreSinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexandreSinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexandreSinger left a comment • 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!

AlexandreSinger commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haydar-c commented Feb 20, 2025

Uh oh!

AlexandreSinger commented Feb 20, 2025

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!

AlexandreSinger commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

vaughnbetz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

haydar-c commented Feb 14, 2025 •

edited by ueqri

Loading

AlexandreSinger left a comment •

edited

Loading

AlexandreSinger commented Feb 20, 2025 •

edited

Loading

AlexandreSinger commented Feb 21, 2025 •

edited

Loading