-
Notifications
You must be signed in to change notification settings - Fork 415
Created draft of new constraints class header file #1535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
42492c7
31b826e
cbaf198
29d9bf5
85f1c10
131eca9
4c93f5f
92f267b
ff1c22d
ff95d41
c3ace92
37ad863
687105b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
#include "partition.h" | ||
#include <algorithm> | ||
#include <vector> | ||
|
||
PartitionId Partition::get_partition_id() { | ||
return id; | ||
} | ||
|
||
void Partition::set_partition_id(PartitionId _part_id) { | ||
id = _part_id; | ||
} | ||
|
||
std::string Partition::get_name() { | ||
return name; | ||
} | ||
|
||
void Partition::set_name(std::string _part_name) { | ||
name = _part_name; | ||
} | ||
|
||
void Partition::add_to_atoms(AtomBlockId atom_id) { | ||
atom_blocks.push_back(atom_id); | ||
} | ||
|
||
bool Partition::contains_atom(AtomBlockId atom_id) { | ||
bool contains_atom = false; | ||
if (std::find(atom_blocks.begin(), atom_blocks.end(), atom_id) != atom_blocks.end()) { | ||
contains_atom = true; | ||
} | ||
return contains_atom; | ||
} | ||
|
||
std::vector<AtomBlockId> Partition::get_atoms() { | ||
return atom_blocks; | ||
} | ||
|
||
PartitionRegions Partition::get_part_regions() { | ||
return part_regions; | ||
} | ||
|
||
void Partition::set_part_regions(PartitionRegions pr) { | ||
part_regions = pr; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
#ifndef PARTITION_H | ||
#define PARTITION_H | ||
|
||
#include "vtr_strong_id.h" | ||
#include "region.h" | ||
#include "partition_regions.h" | ||
|
||
/** | ||
* @file | ||
* @brief This file defines the data for a partition: a grouping of atoms that are constrained to a portion of an FPGA. A partition defines | ||
* the atoms that are assigned to it, and the locations in which they can be placed. The locations are a union of rectangles in x, y, | ||
* sub_tile space. Common cases include a single rectangle or a single x, y, sub_tile (specific location). More complex partitions | ||
* with L, T or other shapes can be created with a union of multiple rectangles. | ||
*/ | ||
|
||
//Type tag for PartitionId | ||
struct partition_id_tag; | ||
|
||
//A unique identifier for a partition | ||
typedef vtr::StrongId<partition_id_tag> PartitionId; | ||
|
||
class Partition { | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public: | ||
vaughnbetz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
PartitionId get_partition_id(); | ||
void set_partition_id(PartitionId _part_id); | ||
|
||
std::string get_name(); | ||
void set_name(std::string _part_name); | ||
|
||
//add an id to the atom_blocks vector | ||
void add_to_atoms(AtomBlockId atom_id); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels like it should be named The description of the method describes the contents of the method, which is not helpful. If someone wanted to understand the implementation, they can read the code. The method comment should describe the why more than the how. Couple example questions that are relevant to this method:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Is there a use case for providing a range of atoms, versus one at a time? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good points. Adding an atom a second time doesn't really make sense; we could check and not add it, but that may get slow with a vector. Could have a validate() method in vpr_constraints.h that checks for conditions like this efficiently (same atom in a partition twice, same atom in two different partitions) and prints a useful error message. I don't think having an atom in two partitions makes sense. It will complicate the data structures and the use case isn't very clear to me. It makes it possible to have wildcards that overlap, e.g.:
That would only make sense if partition 2 is constrained to a region that is a subset of partition 1. So you could make cases where this kind-of makes sense, but documenting it is going to be pain and it makes more complex code for not much gain. Instead we could say: Either A or B seems like a reasonable choice. Sarah, I suggest trying such a constraint in Quartus to see what it does; we don't have to do the same thing but it's useful to know what Quartus chose in such a case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to be clear, my examples where not about the implementation, but about the documentation and intention behind how to use the method. If I find the "add_to_atoms" method, and I want to call it, what is valid, what isn't, etc. Reading the implementation won't tell me answers to these questions, especially if the validation is done on the complete set of data. It is totally fine in the comment to say, "see this other method that maintains the following invariant, etc". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I'll check how Quartus handles this. In the case of choosing option B, would we then specify to users in documentation that their constraints should go from more general to more specific? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sarah: yes, we'd document the behavior and why (no matter what is chosen). |
||
|
||
//check if a given atom is in the partition | ||
vaughnbetz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
bool contains_atom(AtomBlockId atom_id); | ||
|
||
//get the atom_blocks vector | ||
std::vector<AtomBlockId> get_atoms(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This returns a new copy of the Method should be const? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Yes, should be const, and should just return a reference. Haven't used array_view; may be more appropriate still. |
||
|
||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
//set the union of regions for this partition; | ||
void set_part_regions(PartitionRegions pr); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a nit, but I think it is worth consider. In general, plural methods like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To illustrate what I'm talking about, consider There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could rename PartitionRegions as PartitionRegion, and PartitionRegion as RectPlusSubtile (or something like that). Now PartitionRegion is singular, indicating it stores a single (possibly complex) region, and RectPlusSubtile indicates the basic elements used to describe a (possibly composite) region. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, Region i.e. RectPlusSubtile does not store a composite region - it stores a single rectangular area and optionally a subtile. So changing the name would specify this. I do think it would be better to rename PartitionRegions to something non-plural since it is a collection of regions belonging to one particular partition. PartitionCollection could work, I could comment that the class stores the collection of regions (RectPlusSubtile's) belonging to the partition. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think PartitionCollection is also the wrong name. This is really storing the (possibly complex) region that goes with one partition. So it really should be called PartitionRegion. PartitionRegions and PartitionCollection both let part of the implementation (currently a union of simple rectangles) leak through the name, which isn't great. If we someday change to more complex region representations like those Keith suggests, PartitionRegion still works as a name, but PartitionCollection doesn't work as well. |
||
|
||
//get the union of regions of this partition | ||
PartitionRegions get_part_regions(); | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
private: | ||
PartitionId id; //unique id for this partition | ||
std::string name; //name of the partition, name will be unique across partitions | ||
std::vector<AtomBlockId> atom_blocks; //atoms that belong to this partition, each atom should only belong to one partition | ||
PartitionRegions part_regions; //the union of regions that the partition can be placed in | ||
}; | ||
|
||
#endif /* PARTITION_H */ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
#include "partition_regions.h" | ||
|
||
PartitionRegions PartitionRegions::get_intersection(PartitionRegions region) { | ||
//region is a vector of regions, part_regions is another vector of regions | ||
//have to intersect the regions and return their intersection | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
PartitionRegions pr; | ||
Region intersect_region; | ||
bool regions_intersect; | ||
for (int i = 0; i < partition_regions.size(); i++) { | ||
for (int j = 0; j < region.partition_regions.size(); j++) { | ||
regions_intersect = partition_regions[i].do_regions_intersect(region.partition_regions[j]); | ||
if (regions_intersect) { | ||
intersect_region = partition_regions[i].regions_intersection(region.partition_regions[j]); | ||
pr.partition_regions.push_back(intersect_region); | ||
} | ||
} | ||
} | ||
|
||
return pr; | ||
} | ||
|
||
bool PartitionRegions::intersects(PartitionRegions region) { | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
//fill in check for intersection | ||
bool test; | ||
return test; | ||
} | ||
|
||
PartitionRegions PartitionRegions::get_intersect(const AtomBlockId atom_id) { | ||
//fill in to get intersection | ||
PartitionRegions pr; | ||
return pr; | ||
} | ||
|
||
void PartitionRegions::add_to_part_regions(Region region) { | ||
partition_regions.push_back(region); | ||
} | ||
|
||
std::vector<Region> PartitionRegions::get_partition_regions() { | ||
return partition_regions; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
#ifndef PARTITION_REGIONS_H | ||
#define PARTITION_REGIONS_H | ||
|
||
#include "region.h" | ||
|
||
/** | ||
* @file | ||
* @brief This file defines the PartitionRegions class. The PartitionRegions class is used to store the union | ||
* of regions that a partition can be placed in. | ||
*/ | ||
|
||
class PartitionRegions { | ||
public: | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* Returns the intersection of two PartitionRegions vectors that are passed to it. | ||
*/ | ||
PartitionRegions get_intersection(PartitionRegions region); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider making this a static method that consumes two arguments? |
||
|
||
/** | ||
* A test for intersection of the PartitionRegions of two blocks | ||
*/ | ||
bool intersects(PartitionRegions region); | ||
|
||
/** | ||
* Takes Atom Id returns intersection of atom's PartitionRegions with the PartitionRegions of the object | ||
*/ | ||
PartitionRegions get_intersect(const AtomBlockId atom_id); | ||
|
||
//method to add to the partition_regions vector | ||
void add_to_part_regions(Region region); | ||
|
||
//method to get the partition_regions vector | ||
std::vector<Region> get_partition_regions(); | ||
|
||
private: | ||
std::vector<Region> partition_regions; //vector of regions that a partition can be placed in | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure a vector of Region is the right data structure here. I feel like you want a spatial tree, e.g. : You probably want to prototype a couple different data structures balancing simplicity of implementation, data size overhead, etc. At a minimum, I believe that PartitionRegion needs a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Common case is that the regions are simple (0 or 1 rectangle will be the dominant cases). T-shaped and L-shaped will be possible but relatively rare. So I think a k-d tree etc. is overkill and will be slower and more complex for the common case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given your statement, should we consider a dynamic dispatch approach here? Where the simple 0/1 case is implemented without the vector at all, and the more complicated case gets a more complicated object? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could. I'd leave it simple for now though, and based on future need we can update if we hit cpu bottlenecks, memory footprint etc. The class interface should hide all that from the callers, so re-coding would be limited. |
||
}; | ||
|
||
#endif /* PARTITION_REGIONS_H */ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
#include "region.h" | ||
|
||
//sentinel value for indicating that a subtile has not been specified | ||
int NO_SUBTILE = -1; | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Region::Region() { | ||
sub_tile = NO_SUBTILE; | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
int Region::get_xmin() { | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return region_bounds.xmin(); | ||
} | ||
|
||
int Region::get_xmax() { | ||
return region_bounds.xmax(); | ||
} | ||
|
||
int Region::get_ymin() { | ||
return region_bounds.ymin(); | ||
} | ||
|
||
int Region::get_ymax() { | ||
return region_bounds.ymax(); | ||
} | ||
|
||
void Region::set_region_rect(int _xmin, int _ymin, int _xmax, int _ymax) { | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
region_bounds.set_xmin(_xmin); | ||
region_bounds.set_xmax(_xmax); | ||
region_bounds.set_ymin(_ymin); | ||
region_bounds.set_ymax(_ymax); | ||
} | ||
|
||
int Region::get_sub_tile() { | ||
return sub_tile; | ||
} | ||
|
||
void Region::set_sub_tile(int _sub_tile) { | ||
sub_tile = _sub_tile; | ||
} | ||
|
||
bool Region::do_regions_intersect(Region region) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to intersect, should probably make this |
||
bool intersect = true; | ||
|
||
int x_min_1 = region.get_xmin(); | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
int x_min_2 = region_bounds.xmin(); | ||
int int_x_min = std::max(x_min_1, x_min_2); | ||
|
||
int y_min_1 = region.get_ymin(); | ||
int y_min_2 = region_bounds.ymin(); | ||
int int_y_min = std::max(y_min_1, y_min_2); | ||
|
||
int x_max_1 = region.get_xmax(); | ||
int x_max_2 = region_bounds.xmax(); | ||
int int_x_max = std::min(x_max_1, x_max_2); | ||
|
||
int y_max_1 = region.get_ymax(); | ||
int y_max_2 = region_bounds.ymax(); | ||
int int_y_max = std::min(y_max_1, y_max_2); | ||
|
||
//check if rectangles dimensions are invalid | ||
if (int_x_min > int_x_max || int_y_min > int_y_max) { | ||
intersect = false; | ||
} | ||
|
||
return intersect; | ||
} | ||
|
||
Region Region::regions_intersection(Region region) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rename to intersection, and make it a global function at the bottom of the header file (outside class): |
||
Region intersect; | ||
|
||
int x_min_1 = region.get_xmin(); | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
int x_min_2 = region_bounds.xmin(); | ||
int int_x_min = std::max(x_min_1, x_min_2); | ||
|
||
int y_min_1 = region.get_ymin(); | ||
int y_min_2 = region_bounds.ymin(); | ||
int int_y_min = std::max(y_min_1, y_min_2); | ||
|
||
int x_max_1 = region.get_xmax(); | ||
int x_max_2 = region_bounds.xmax(); | ||
int int_x_max = std::min(x_max_1, x_max_2); | ||
|
||
int y_max_1 = region.get_ymax(); | ||
int y_max_2 = region_bounds.ymax(); | ||
int int_y_max = std::min(y_max_1, y_max_2); | ||
|
||
intersect.set_region_rect(int_x_min, int_y_min, int_x_max, int_y_max); | ||
|
||
return intersect; | ||
} | ||
|
||
bool Region::locked() { | ||
bool locked = false; | ||
bool x_matches = false; | ||
bool y_matches = false; | ||
|
||
if (region_bounds.xmin() == region_bounds.xmax()) { | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
x_matches = true; | ||
} | ||
|
||
if (region_bounds.ymin() == region_bounds.ymax()) { | ||
y_matches = true; | ||
} | ||
|
||
if (x_matches && y_matches && sub_tile != NO_SUBTILE) { | ||
locked = true; | ||
} | ||
|
||
return locked; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
#ifndef REGION_H | ||
#define REGION_H | ||
|
||
#include "globals.h" | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* @file | ||
* @brief This file defines the Region class. The Region class stores the data for each constraint region. | ||
* This includes the bounds of the region rectangle and the sub_tile bounds. To lock a block down to a specific location, | ||
* when defining the region specify xmin = xmax, ymin = ymax, sub_tile_min = sub_tile_max | ||
* | ||
*/ | ||
|
||
//sentinel value for indicating that a subtile had not been specified | ||
extern int NO_SUBTILE; | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
class Region { | ||
public: | ||
Region(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add @brief on these methods too so they show up in Doxygen (then build Doxygen to the vtr docs and make sure it shows up). |
||
|
||
//vtr::Rect get_region_rect(); | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
int get_xmin(); | ||
int get_xmax(); | ||
int get_ymin(); | ||
int get_ymax(); | ||
void set_region_rect(int _xmin, int _ymin, int _xmax, int _ymax); | ||
|
||
int get_sub_tile(); | ||
|
||
void set_sub_tile(int _sub_tile); | ||
|
||
//function to see if two regions intersect anywhere | ||
bool do_regions_intersect(Region region); | ||
|
||
//returns the coordinates of the intersection of two regions | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Region regions_intersection(Region region); | ||
|
||
//function to check whether a block is locked down to a specific x, y, subtile location | ||
//can be called by the placer to see if the block is locked down | ||
bool locked(); | ||
|
||
private: | ||
//may need to include zmin, zmax for future use in 3D FPGA designs | ||
vtr::Rect<int> region_bounds; //xmin, ymin, xmax, ymax inclusive | ||
int sub_tile; //users will optionally select a subtile, will select if they want to lock down block to specific location | ||
}; | ||
|
||
#endif /* REGION_H */ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
#include "vpr_constraints.h" | ||
|
||
void VprConstraints::add_constrained_atom(const AtomBlockId blk_id, const PartitionId partition) { | ||
constrained_atoms.insert({blk_id, partition}); | ||
} | ||
|
||
PartitionId VprConstraints::get_atom_partition(AtomBlockId blk_id) { | ||
PartitionId part_id; | ||
part_id = constrained_atoms.at(blk_id); | ||
sfkhalid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return part_id; | ||
} | ||
|
||
void VprConstraints::add_partition(Partition part) { | ||
partitions.push_back(part); | ||
} | ||
|
||
vtr::vector<PartitionId, Partition> VprConstraints::get_partitions() { | ||
return partitions; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style should be consistent with other code. I haven't seen underscore prefixes for parameters elsewhere, and an underscore suffix (e.g.
id_
) is typically used for member variables.