vpr: Add fix_clusters option to allow users to lock down any block to… #1433

sfkhalid · 2020-07-16T14:33:32Z

… an x, y, subtile location

VPR had the option fix_pins to allow users to fix I/O pads to certain locations. The fix_clusters option was added to allow them to fix any block (I/O or otherwise) to an x, y, subtile location. It has the same arguments that fix_pins has - free, random, and user. Using the user argument entails providing a .pads file, just like fix_pins.

Description

Added fix_clusters in the place group options in the read_options.cpp and argparse_test.cpp files.

Related Issue

The related issue is: Add Placement Constraints to VPR #932
#932

How Has This Been Tested?

The fix_clusters option was tested with running vpr with an architecture file that has clbs with a block capacity of greater than one. The vtr_reg_basic and vtr_reg_strong tests were also run.

Types of changes

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

Checklist:

[x ] 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

… an x, y, subtile location

kmurray · 2020-07-18T21:35:11Z

Question for @sfkhalid and @vaughnbetz. Is this the same or different from #1189?

vaughnbetz · 2020-07-18T22:56:19Z

Yes, it's the same. I forgot that one got merged, but Sarah found the functionality works (which makes sense, since Keith added it). She's breaking up the options:
--fix_pins will now only have free(default) and random options.
--fix_clusters will take a file argument, and will specify any placement blocks that should be locked down.
pad_loc_file will be renamed as something new: I suggest constraints.place as a reasonable name since it uses the .place format.
Sarah, inside the code and documentation you should rename pad_loc_file to this new name, and make sure comments and variables names are changed to reflect the new functionality as appropriate.
Sarah is also going to add regtests for the new ability to lock down arbitrary blocks and a regtest to ensure that invalid constraints cause an error.

…ters option have the option to provide a constraints file

vaughnbetz · 2020-07-27T22:09:02Z

Sarah, there appear to be conflicts -- can you check and resolve?
Also, please remove the dead enums we talked about last week, and any other dead code. If fix_pins USER is gone, all mention of it should now be removed from the code.

sfkhalid · 2020-07-27T22:14:06Z

Sarah, there appear to be conflicts -- can you check and resolve?
Also, please remove the dead enums we talked about last week, and any other dead code. If fix_pins USER is gone, all mention of it should now be removed from the code.

Hi Vaughn, when I check this pull request it says that there are no conflicts with the base branch, are you seeing a different message? Also, I'll be sure to commit the code with the USER option completely removed tonight.

vaughnbetz · 2020-07-27T22:24:51Z

Looks like it can't be rebased, but it can be directly merged (which would only affect the commit history, making it a bit harder to interpret, which is OK for a straightforward change like this). I see this message:

This branch cannot be rebased due to conflicts
Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch.

but if I switch to creating a merge commit it is OK.

…on --fix_pins

…t functionality of fix_clusters option

…or the fix_clusters reg test but was previously ignored

…ionality has been replaced in the code by fix_clusters

vaughnbetz

Make changes listed here, do PR.
Also take a look at how the blocks are locked down (in initial_place.cpp). Is it clean? If not, could lock them down when reading the constraints_file. If can be cleaned up, do another PR for that.
It looks like the placement file and constraint file reading could be refactored to have one common routine. Check if that would be cleaner, and if so do a 3rd PR.

would have to refactor read_place to call a couple of subfunctions: read_place_header and read_place_body
constraints file would also call read_place_body
read_place_body should get the error checking from read_constraints_file (stronger checker, assumes user could have typed random stuff)
parser from read_place_body looks nicer --> probably build off it.
Check that the cluster_netlist.find_block(block_name) is fast (should be using a map or hash table (unordered_map). read_place_constraints is using my hand-written hash table, which isn't as good (doesn't resize for big circuits).
Depending on what you did in ODIN II: Instantiating more than one module with dual_port_ram inside fails #2, you might also have to pass in a flag saying whether you are reading as a constraint (set block.locked flag) or a placement file (don't set).

Test using a placement file and a constraint file together. What happens? Current code looks like it will error in odd ways unless the overall control code avoids parsing the constraint file in that case (which would be fine).

vaughnbetz · 2020-07-30T15:14:11Z

libs/EXTERNAL/libargparse/argparse_test.cpp

@@ -302,6 +303,13 @@ int main(
                  " or a file specifying the pad locations.")
            .default_value("off")
            .show_in(argparse::ShowIn::HELP_ONLY);
+    place_grp.add_argument(args.fix_clusters, "--fix_clusters")
+            .help("Fixes block locations during placement."
+                  " Can be 'random' for a random initial assignment,"


Need to remove random option.

vaughnbetz · 2020-07-30T15:16:04Z

vpr/src/base/SetupVPR.cpp

@@ -503,8 +503,10 @@ static void SetupPlacerOpts(const t_options& Options, t_placer_opts* PlacerOpts)

    PlacerOpts->place_algorithm = Options.PlaceAlgorithm;

+    PlacerOpts->constraints_file = Options.constraints_file;
    PlacerOpts->pad_loc_file = Options.pad_loc_file;


Remove pad_loc_file

vaughnbetz · 2020-07-30T15:19:08Z

vpr/src/base/SetupVPR.cpp

    PlacerOpts->pad_loc_file = Options.pad_loc_file;
    PlacerOpts->pad_loc_type = Options.pad_loc_type;
+    PlacerOpts->block_loc_type = Options.block_loc_type;


block_loc_type isn't a great name; not obvious it goes with the constraints file.
But probably better still to simply have a NULL or empty constraints_file name mean no location constraints.

I think the best thing is to delete block_loc_type entirely and use a NULL constraints file name to mean no constraints. Comment it when you do the test for a NULL constraints file (means none specified).

vaughnbetz · 2020-07-30T15:29:40Z

vpr/src/base/ShowSetup.cpp

            default:
                VPR_FATAL_ERROR(VPR_ERROR_UNKNOWN, "Unknown I/O pad location type\n");
        }

+        VTR_LOG("PlacerOpts.block_loc_type: ");


Make more user friendly.
should just be PlacerOpts.constraint_file: fname

At the same time, put a blank line before the first placer option (or after last packer option). Other groups have a blank line between them; placer and packer should too.

vaughnbetz · 2020-07-30T15:31:50Z

vpr/src/base/read_options.cpp

+        .help(
+            "Fixes block locations during placement. Valid options:\n"
+            " * path to a file specifying block locations (.place format with block locations specified).")
+        .default_value("not_locked")


Update this to just have a filename, default is empty string (no constraint file).

vaughnbetz · 2020-07-30T15:46:16Z

vpr/src/base/vpr_types.h

@@ -816,8 +821,10 @@ struct t_annealing_sched {
 * place_chan_width:  The channel width assumed if only one placement is     *
 *                    performed.                                             *
 * pad_loc_type:  Are pins FREE, fixed randomly, or fixed from a file.       *
- * pad_loc_file:  File to read pin locations form if pad_loc_type            *
- *                     is USER.                                              *
+ * block_loc_type: Are blocks fixed from a file.                             *


vaughnbetz · 2020-07-30T15:46:26Z

vpr/src/base/vpr_types.h

+ * block_loc_type: Are blocks fixed from a file.                             *
+ * constraints_file:  File to read block locations from if block_loc_type    *
+ *                     is LOCKED.                                            *
+ * pad_loc_file: File to read pad locations from if pad_loc_type is USER.    *


remove pad_loc_file

vaughnbetz · 2020-07-30T15:46:45Z

vpr/src/base/vpr_types.h

@@ -870,6 +877,8 @@ struct t_placer_opts {
    float place_cost_exp;
    int place_chan_width;
    enum e_pad_loc_type pad_loc_type;


remove block_loc_type

vaughnbetz · 2020-07-30T15:48:12Z

vpr/src/place/initial_placement.cpp

-        read_user_pad_loc(pad_loc_file);
+    /*If the user specified block locations using a constraints file, read those locations in here*/
+    if (block_loc_type == LOCKED) {
+        read_user_block_loc(constraints_file);


Change to just call read_user_block_loc (can have it return if constraints_file is NULL or an empty string). Or could check before calling it with this if ().

vaughnbetz · 2020-07-30T15:48:47Z

vpr/src/place/initial_placement.h

@@ -3,7 +3,6 @@

 #include "vpr_types.h"

-void initial_placement(enum e_pad_loc_type pad_loc_type,
-                       const char* pad_loc_file);
+void initial_placement(enum e_pad_loc_type pad_loc_type, enum e_block_loc_type block_loc_type, const char* constraints_file);


remove block_loc_type

vpr: Add fix_clusters option to allow users to lock down any block to…

96ccbb8

… an x, y, subtile location

probot-autolabeler bot added external_libs lang-cpp C/C++ code tests VPR VPR FPGA Placement & Routing Tool labels Jul 16, 2020

sfkhalid and others added 4 commits July 19, 2020 18:45

Pushing formatted code to adding_fix_clusters_option branch

8e29f41

Make fix_pins option have free and random arguments and make fix_clus…

4af32fa

…ters option have the option to provide a constraints file

Merge branch 'master' into adding_fix_clusters_option

40a8d08

Pushing formatted code to branch

8f11883

sfkhalid added 4 commits July 28, 2020 20:43

Removing dead code pertaining to the former USER paramter of the opti…

877adb2

…on --fix_pins

Adding a reg test called strong_fix_clusters to vtr_reg_strong to tes…

5822076

…t functionality of fix_clusters option

Adding apex2_block_locations constraints file since it is necessary f…

15ffd79

…or the fix_clusters reg test but was previously ignored

Removing files for strong_fix_pins_pad_file reg test since that funct…

9f0515f

…ionality has been replaced in the code by fix_clusters

probot-autolabeler bot added the VTR Flow VTR Design Flow (scripts/benchmarks/architectures) label Jul 29, 2020

sfkhalid merged commit 22e69b1 into master Jul 29, 2020

sfkhalid deleted the adding_fix_clusters_option branch July 29, 2020 15:45

vaughnbetz reviewed Jul 30, 2020

View reviewed changes

vpr: Add fix_clusters option to allow users to lock down any block to… #1433

vpr: Add fix_clusters option to allow users to lock down any block to… #1433

Uh oh!

Conversation

sfkhalid commented Jul 16, 2020

Description

Related Issue

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

kmurray commented Jul 18, 2020

Uh oh!

vaughnbetz commented Jul 18, 2020

Uh oh!

vaughnbetz commented Jul 27, 2020

Uh oh!

sfkhalid commented Jul 27, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

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!

Uh oh!

vaughnbetz commented Jul 27, 2020 •

edited

Loading

vaughnbetz left a comment •

edited

Loading