Refactored read_place and read_user_block_loc functions into two new … #1490

sfkhalid · 2020-08-19T21:02:47Z

…functions

Two new functions - read_place_header and read_place_body - were created to replace read_place and read_user_block_loc. Read_place and read_user_block_loc had some similar code that could be condensed into the same function, which was done in read_place_body.

Description

Read_place_header now reads in the first two lines of a place file, this used to be done in read_place. Read_place_body reads in either a place file (minus the header) or a constraints file, which used to be done in two separate functions: read_place and read_user_block_loc.

How Has This Been Tested?

Tested with fix_clusters option (which calls read_place_body) on individual circuits. Also tested that functions work when loading a placement file. Also checked that it passes basic and strong reg tests.

Checklist:

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

…functions

vaughnbetz · 2020-08-20T16:27:46Z

Discussed and reviewed in our meeting. Please apply the changes discussed and update the commits. Some of the changes:

have read_place and read_constraints top level functions, to simplify callers use.
Comment how to use them and any things they expect in the header file, using Doxygen-compatible /** comments. Right now the file readers overwrite any existing placement (set .x == OPEN I think). Check if this is necessary; if so comment it as a side effect, if not remove that behaviour.
Avoid use of .x == OPEN to count how many times a block appeared in a file. Use a vector instead (after checking that the block id space is dense, from 0 to num_blocks-1 or so.
Should remove duplicate header processing code in read_place_body by passing a file pointer in (has already gone past the header)
Should check subtile legality
Check if constraints need to set the grid locations or just the block.x,.y etc. Avoid setting grid for place and constraint uses if possible, to minimize differences.
Should print out "Reading file ..." and "Finished reading file ..." for both placement and constraint files.
Summarize your testing of error conditions.
- Missing header, duplicate header, header in wrong spot, ...
- Missing block, duplicate block, name not found for block, x,y, subtile out of range, two blocks placed at same grid location. All the errors should be caught in the file reader (for both place and constraint files) except two blocks at same grid location, which hopefully is caught in check_place right after file reading (but check this).
- Check what happens when you have a placement and constraint file that don't agree on a block location. Overwrite the location (constraint file wins), or error (check compatiable). Both are reasonable behaviours; could do overwrite (with a message if possible).

vaughnbetz · 2020-08-20T16:34:59Z

vpr/src/base/read_place.cpp

+        VTR_LOG("Successfully read %s.\n", place_file);
+        VTR_LOG("\n");
+    }
+    place_ctx.placement_id = vtr::secure_digest_file(place_file);


This is a bug; shouldn't generate the signature (used to verify that a routing file uses a compatible placement if we later read in that routing file) for the placement if we're not reading a placement file.

…d_place_header and read_place_body

sfkhalid · 2020-08-27T14:25:36Z

Read_place tests done: duplicate blocks listed, block out of range of grid dimensions, block locations incompatible, subtile number out of range, incomplete/invalid lines, two blocks placed at same location (caught by check_place)
Invalid block name - not sure how to check for this. Previously a hash table was used to store block names and IDs and checked if names were valid by seeing if they were in the hash table.
Testing place file and constraints file at same time - constraints file is not read at all because read_place is only called in a situation where placer_opts.doPlacement == STAGE_LOAD

sfkhalid · 2020-08-27T15:27:09Z

vpr/src/base/read_place.cpp

-            place_ctx.block_locs[blk_id].loc.x = block_x;
-            place_ctx.block_locs[blk_id].loc.y = block_y;
-            place_ctx.block_locs[blk_id].loc.sub_tile = sub_tile_index;
+            seen_grid_dimensions = true; //if you have read the grid dimensions you are done reading the place file header


Fix comment at this line

sfkhalid · 2020-08-27T15:29:38Z

vpr/src/base/read_place.cpp

+
+/**
+ * This function reads the header (first two lines) of a placement file.
+ * It checks whether the packed netlist file that generated the placement matches the current netlist file.


Say what the header is - two lines in any order that give netlist and grid files used to generate placement. Also mention grid gives an error if it doesn't match

sfkhalid · 2020-08-27T15:30:19Z

vpr/src/base/read_place.cpp

- * All blocks specified in this file will be locked down at their specified x, y, subtile locations.
- * (Will not be moved by optimizers during placement
+/**
+ * This function reads either the body of a placement file or a constraints file.


Add that constraints file is same format as place

sfkhalid · 2020-08-27T15:32:03Z

vpr/src/base/read_place.cpp

+            //need to lock down blocks  and mark grid block usage if it is a constraints file
+            if (!is_place_file) {
+                place_ctx.block_locs[blk_id].is_fixed = true;
+                place_ctx.grid_blocks[block_x][block_y].blocks[sub_tile_index] = blk_id;


Try setting grid all the time, check that it works

sfkhalid · 2020-08-27T15:33:56Z

vpr/src/base/read_place.cpp


-    ptr = vtr::fgets(buf, vtr::bufsize, fp);
+    std::vector<ClusterBlockId> seen_blocks;


Comment what this is

should be a vtr::vector_map<ClusterBlockId, int>

sfkhalid · 2020-08-27T15:35:06Z

vpr/src/base/read_place.cpp

-        ClusterBlockId bnum(h_ptr->index);
-        int i = xtmp;
-        int j = ytmp;
+            ClusterBlockId blk_id = cluster_ctx.clb_nlist.find_block(block_name);


find what this returns if it doesn't find the block name - use for invalid block name check

sfkhalid · 2020-08-27T15:46:25Z

vpr/src/base/read_place.cpp

-        place_ctx.block_locs[bnum].loc.y = j; /* We need to set .x only as a done flag.         */
-        place_ctx.block_locs[bnum].loc.sub_tile = k;
-        place_ctx.block_locs[bnum].is_fixed = true;
+            if (place_ctx.block_locs.size() != cluster_ctx.clb_nlist.blocks().size()) {


check if we need this. if we do, should be out of the loop

sfkhalid · 2020-08-27T15:49:01Z

vpr/src/base/read_place.cpp

-            continue;
-        }
+            //add to vector of blocks that have been seen
+            seen_blocks.push_back(blk_id);


change once you refactor to vector map

sfkhalid · 2020-08-27T15:51:20Z

vpr/src/base/read_place.cpp

+            //Unrecognized
+            vpr_throw(VPR_ERROR_PLACE_F, place_file, lineno,
+                      "Invalid line '%s' in file",
+                      line.c_str());
        }
    }



add in check for place file to see if it has checked all blocks

…error messages

sfkhalid · 2020-09-03T16:35:11Z

vpr/src/base/read_place.cpp

+                if (atom_blk_id == AtomBlockId::INVALID()) {
+                    VPR_THROW(VPR_ERROR_PLACE, "Block %s has an invalid name.\n", c_block_name);
+                } else {
+                    blk_id = atom_ctx.lookup.atom_clb(atom_blk_id);


add comment explaining this

sfkhalid · 2020-09-03T16:41:15Z

vpr/src/base/read_place.cpp

+                VPR_THROW(VPR_ERROR_PLACE, "Attempt to place block %s with ID %d at illegal location (%d, %d). \n", c_block_name, blk_id, block_x, block_y);
+            }
+
+            //need to lock down blocks  and mark grid block usage if it is a constraints file


explain why you need to do this (if place file, initial placement does instead)

sfkhalid · 2020-09-03T16:41:52Z

vpr/src/base/read_place.cpp

+            }
+
+            //mark the block as seen
+            seen_blocks[blk_id] = 1;


make ++ instead of 1

sfkhalid · 2020-09-03T16:42:47Z

vpr/src/base/read_place.cpp

-        if (place_ctx.block_locs[blk_id].loc.x == OPEN) {
-            VPR_THROW(VPR_ERROR_PLACE_F, constraints_file, 0,
-                      "Block %s location was not specified in the constraints file.\n", cluster_ctx.clb_nlist.block_name(blk_id).c_str());
+    //For place files, check that all blocks have been read


For constraints, don't need to include all add to comment

sfkhalid · 2020-09-03T16:43:20Z

vpr/src/base/read_place.cpp

-    free_hash_table(hash_table);
-    VTR_LOG("Successfully read %s.\n", constraints_file);
-    VTR_LOG("\n");
+    if (is_place_file) {


add comment to explain why

for entire place file, want to make hash so we can write into routing file for later error checking, dont need for constraint

sfkhalid · 2020-09-03T16:46:19Z

vpr/src/base/read_place.h

@@ -1,16 +1,25 @@
 #ifndef READ_PLACE_H
 #define READ_PLACE_H

+/**
+ * This function is used to read a placement file.


so that we can skip placement, check netlist file, grid dimensions, if verify flag is set must give error

sfkhalid · 2020-09-03T16:46:36Z

vpr/src/base/read_place.h

@@ -1,16 +1,25 @@
 #ifndef READ_PLACE_H
 #define READ_PLACE_H

+/**
+ * This function is used to read a placement file.
+ */
 void read_place(


doesnt need to be passed is_place_file

sfkhalid · 2020-09-03T16:46:59Z

vpr/src/base/read_place.h

+/**
+ * This function is used to read a constraints file that specifies the desired locations of blocks.
+ */
+void read_constraints(


doesnt need is_place_file to be passed

vaughnbetz · 2020-09-03T17:20:48Z

One more update to make: make --fix_pins give a good message pointing at --fix_clusters if the user tries to specify a file name.

sfkhalid · 2020-09-04T21:53:50Z

Earlier we said that if a location is specified twice for the same block (ex. by two different primitives in the same cluster) we don't have to give an error as long as the locations don't conflict. But in this case (location of a block specified twice with no conflict) check_place gives an error later on during the placement consistency check. Given this, should I give an error in read_place_body if a location is specified twice for the same cluster?

vaughnbetz · 2020-09-04T22:02:28Z

What is the error from check_place?
I am guessing this line is the culprit, if you didn't change it (guard it with an if so that usage is only increased the first time a block location is read):
place_ctx.grid_blocks[block_x][block_y].usage++;
If you increment usage (how many blocks are at that grid location) each time you read a primitive in the same cluster, you will corrupt the placement state and fail check_place. But avoiding that is not hard, with small code changes in read_place.

sfkhalid · 2020-09-04T22:12:02Z

What is the error from check_place?
I am guessing this line is the culprit, if you didn't change it (guard it with an if so that usage is only increased the first time a block location is read):
place_ctx.grid_blocks[block_x][block_y].usage++;
If you increment usage (how many blocks are at that grid location) each time you read a primitive in the same cluster, you will corrupt the placement state and fail check_place. But avoiding that is not hard, with small code changes in read_place.

Yes, that was the problem. Adding the if statement as you suggested fixed it

… reg tests

sfkhalid · 2020-09-08T13:55:03Z

Merging with one failure in Travis CI - Branch. This failure is just the Python Lint failure that has been popping up.

Refactored read_place and read_user_block_loc functions into two new …

c96a78a

…functions

probot-autolabeler bot added lang-cpp C/C++ code VPR VPR FPGA Placement & Routing Tool labels Aug 19, 2020

sfkhalid added 2 commits August 20, 2020 00:09

Added error checking to read_place_body

926ac26

Ran make format to format code

c512c70

vaughnbetz reviewed Aug 20, 2020

View reviewed changes

sfkhalid added 2 commits August 26, 2020 20:10

Added read_place and read_constraints functions and made fixes to rea…

7e4fb4a

…d_place_header and read_place_body

Got rid of line printing std::string block_name to fix compiler warning

d3a5046

sfkhalid commented Aug 27, 2020

View reviewed changes

sfkhalid added 2 commits August 31, 2020 20:20

Changed method of detecting duplicate blocks and added block name to …

257b53e

…error messages

Added ability to lock down clusters with primitive names

724f717

sfkhalid commented Sep 3, 2020

View reviewed changes

vpr/src/base/read_place.cpp Outdated

}

//mark the block as seen

seen_blocks[blk_id] = 1;

Copy link

Contributor Author

sfkhalid Sep 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make ++ instead of 1

sfkhalid commented Sep 3, 2020

View reviewed changes

Changed comments and created new golden result for one of the nightly…

2102256

… reg tests

probot-autolabeler bot added tests VTR Flow VTR Design Flow (scripts/benchmarks/architectures) labels Sep 7, 2020

Ran make format

5192d2c

sfkhalid merged commit 4b86a9e into master Sep 8, 2020

sfkhalid deleted the refactoring_read_place branch September 8, 2020 13:55


		ptr = vtr::fgets(buf, vtr::bufsize, fp);
		std::vector<ClusterBlockId> seen_blocks;

Refactored read_place and read_user_block_loc functions into two new … #1490

Refactored read_place and read_user_block_loc functions into two new … #1490

Uh oh!

Conversation

sfkhalid commented Aug 19, 2020

Description

How Has This Been Tested?

Checklist:

Uh oh!

vaughnbetz commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfkhalid commented Aug 27, 2020

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!

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!

vaughnbetz commented Sep 3, 2020

Uh oh!

sfkhalid commented Sep 4, 2020

Uh oh!

vaughnbetz commented Sep 4, 2020

Uh oh!

sfkhalid commented Sep 4, 2020

Uh oh!

sfkhalid commented Sep 8, 2020

Uh oh!

Uh oh!

vaughnbetz commented Aug 20, 2020 •

edited

Loading