Added cleaning of a pb after unsuccessful molecule packing #1656

mkurc-ant · 2021-02-03T17:05:55Z

Attempt to fix #1655

litghost

This needs a test.

mkurc-ant · 2021-02-09T11:36:15Z

This needs a test.

mkurc-ant · 2021-02-09T11:45:24Z

I've run vtr_reg_nightly on the same machine. Both for master (@53780c0b9) and this PR. There was no difference in any of the QoR metrics except runtime (this was expected).

metric	baseline	modified (this PR)
total_runtime	1.000	1.010
pack_time	1.000	1.013

litghost

LGTM, waiting for other reviews.

acomodi

LGTM. I think we need to re-run CI as the Nightly one errored out for some reason (link).

vaughnbetz · 2021-02-09T19:57:26Z

vpr/src/pack/cluster.cpp

@@ -1179,6 +1179,64 @@ static void alloc_and_load_pb_stats(t_pb* pb, const int feasible_block_array_siz
 }
 /*****************************************/

+/**
+ * Cleans up a pb after unsuccessful molecule packing
+ */


Good to comment this routine and how it works as much as you can.

What exactly is it cleaning up? When is it safe to call?

vaughnbetz · 2021-02-09T19:59:08Z

LGTM too. My one request is to comment it as much as you can. I realize that may be limited if you don't have full understanding of exactly how / why these structures are set up, but if you give as much insight as you can into why this routine is being called, what it does and when it is safe to call it that would be good.

sfkhalid · 2021-02-09T21:14:26Z

vpr/src/pack/cluster.cpp

@@ -1358,6 +1416,10 @@ static enum e_block_pack_status try_pack_molecule(t_cluster_placement_stats* clu
                        revert_place_atom_block(molecule->atom_block_ids[i], router_data, atom_molecules);
                    }
                }
+
+                /* Placement failed, clean the pb */


To clarify, is the cleanup_pb function supposed to be called when a molecule fails try_pack_molecule for any reason? Or only when it fails during atom placement (try_place_atom_block_rec)? I’m asking because in the issue it says “Add (or fix?) cleanup code that removes child_pbs created during atom placement if it fails for any reason.”

If it is meant to be called when a molecule fails for any reason, it should also be called if the atom fails the detailed routing legality check in try_pack_molecule. (line 1305/1363)

If the cleanup function is only meant to be called if the atom fails during try_place_atom_block_rec , then maybe the call for cleanup_pb should be at the point right after try_place_atom_block_rec is called. So right after the atom placement attempt, you could have a check of the block_pack_status and call the cleanup function if it is needed. Right now, where the cleanup_pb function is called, it could have failed the placement check or the pin feasibility filter check.

You're right. It should be called whenever try_place_atom_block_rec fails. But if you look at the loop that invokes it for each atom you'll see that it terminates immediately on failure:

vtr-verilog-to-routing/vpr/src/pack/cluster.cpp

Line 1241 in 8e00e82

for (i = 0; i < molecule_size && block_pack_status == BLK_PASSED; i++) {

Later in the flow there are calls to some atom placement reversal routines (

vtr-verilog-to-routing/vpr/src/pack/cluster.cpp

Line 1350 in 8e00e82

if (block_pack_status != BLK_PASSED) {

) executed on failure so I figured out that the pb cleanup should be invoked from there. It doesn't matter what is the reason of the atom placement failure. If no pbs were allocated the function simply won't do anything. BTW: it does not touch pbs with already placed atoms.

Diving into the low-level details: The problem I observed with the assertion failure in try_place_atom_block_rec was that for some reason the function was hitting already allocated pbs with their modes set causing a conflict with the mode required by the currently packed atom. Digging deeper I've discovered that the function is called for a pb_graph_node corresponding to that atom. Then it recurses up the hierarchy allocating pbs and setting their modes until the root CLB is reached. But these pbs are never freed between subsequent molecule packing. So if packing succeeds everything is fine but when it fails there are leftover pbs set to specific modes. This may cause mode mismatch if the mode required for the next molecule is different than the last one checked.

I typed "Add (or fix?)..." in the issue as I was surprised that the problem haven't manifested itself until now. I'm still not sure whether I'm adding the actually missing data structure cleanup or if the cleanup is already there and requires fixing and I've missed it.

Ah I see, that makes sense. In that case LGTM.

mkurc-ant · 2021-02-10T16:55:14Z

I added more comments to the cleanup function that I've added.

vaughnbetz · 2021-02-18T15:52:55Z

Looks good to me. Looks like it needs to be updated to match the base branch and then can be merged.
Also looks like vtr_nightly didn't run again. flagging that to @litghost

mkurc-ant · 2021-02-18T16:22:41Z

@vaughnbetz Updated.

Signed-off-by: Maciej Kurc <[email protected]>

Signed-off-by: Alessandro Comodi <[email protected]>

vaughnbetz · 2021-02-25T18:22:16Z

vtr_reg_nightly got stuck again, but based on the testing and QoR data I'm confident this is OK and we should merge it. @litghost: seems like this is another case of a stuck nightly reg.

mkurc-ant · 2021-02-26T09:05:04Z

@vaughnbetz thanks.

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Feb 3, 2021

mkurc-ant force-pushed the cluster_fix branch 2 times, most recently from 369f5d1 to 5c3070c Compare February 3, 2021 17:19

mkurc-ant requested review from acomodi and vaughnbetz February 4, 2021 14:10

litghost requested changes Feb 4, 2021

View reviewed changes

vaughnbetz requested a review from sfkhalid February 4, 2021 18:52

vaughnbetz assigned luujason and unassigned luujason Feb 4, 2021

vaughnbetz requested a review from luujason February 4, 2021 18:58

mkurc-ant force-pushed the cluster_fix branch from af31ac8 to 86060ce Compare February 8, 2021 13:21

litghost approved these changes Feb 9, 2021

View reviewed changes

acomodi approved these changes Feb 9, 2021

View reviewed changes

vaughnbetz reviewed Feb 9, 2021

View reviewed changes

sfkhalid reviewed Feb 9, 2021

View reviewed changes

mkurc-ant force-pushed the cluster_fix branch from a676b2c to c065ae8 Compare February 10, 2021 16:43

mkurc-ant mentioned this pull request Feb 17, 2021

Support for QuickLogic k4_N8 OpenFPGA-based device f4pga/f4pga-arch-defs#2036

Merged

mkurc-ant requested a review from vaughnbetz February 18, 2021 11:45

mkurc-ant force-pushed the cluster_fix branch from 33790e1 to c1411b1 Compare February 25, 2021 11:35

mkurc-ant and others added 4 commits February 25, 2021 12:36

Added cleaning of a pb after unsuccessful molecule packing

cd7d24e

Signed-off-by: Maciej Kurc <[email protected]>

Added a test that triggers the clustering bug being fixed.

c9f43a9

Signed-off-by: Maciej Kurc <[email protected]>

Enchanced code comments.

4acc6ef

Signed-off-by: Maciej Kurc <[email protected]>

vtr_flow: fix python linting

5fe4b9b

Signed-off-by: Alessandro Comodi <[email protected]>

mkurc-ant force-pushed the cluster_fix branch from c1411b1 to 5fe4b9b Compare February 25, 2021 11:36

vaughnbetz merged commit 8dcd60c into verilog-to-routing:master Feb 25, 2021

Added cleaning of a pb after unsuccessful molecule packing #1656

Added cleaning of a pb after unsuccessful molecule packing #1656

Uh oh!

Conversation

mkurc-ant commented Feb 3, 2021

Uh oh!

litghost left a comment

Choose a reason for hiding this comment

Uh oh!

mkurc-ant commented Feb 9, 2021

Uh oh!

mkurc-ant commented Feb 9, 2021

Uh oh!

litghost left a comment

Choose a reason for hiding this comment

Uh oh!

acomodi 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!

vaughnbetz Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

vaughnbetz Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

vaughnbetz commented Feb 9, 2021

Uh oh!

sfkhalid Feb 9, 2021 • 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!

mkurc-ant Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

sfkhalid Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

mkurc-ant commented Feb 10, 2021

Uh oh!

vaughnbetz commented Feb 18, 2021

Uh oh!

mkurc-ant commented Feb 18, 2021

Uh oh!

vaughnbetz commented Feb 25, 2021

Uh oh!

mkurc-ant commented Feb 26, 2021

Uh oh!

Uh oh!

acomodi left a comment •

edited

Loading

sfkhalid Feb 9, 2021 •

edited

Loading