Skip to content

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

Merged
merged 13 commits into from
Oct 27, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions vpr/src/base/partition.cpp
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;
Copy link
Contributor

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.

}

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;
}
52 changes: 52 additions & 0 deletions vpr/src/base/partition.h
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 {
public:
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);
Copy link
Collaborator

@litghost litghost Oct 19, 2020

Choose a reason for hiding this comment

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

This feels like it should be named add_atom or add_atom_to_partition.

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:

  • What happens if the same atom_id is added twice? What happens then?
  • What is the implication of many partitions with few atoms versus fewer partitions with many atoms?
  • What is the implication of an atom being added to two Partition?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

@vaughnbetz vaughnbetz Oct 19, 2020

Choose a reason for hiding this comment

The 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.:

*alu* --> partition 1
*alu|foo --> partition 2

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:
A: assigning the same atom to two partitions is an error (and then you have to check during parsing)
B: The last assignment to an atom wins (removes any prior assignment). That means if you code some relatively loose and some relatively specific constraints, you would order them from loose to specific to take advantage of this behaviour.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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".

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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).
Keith: agreed, this should be documented for the function in the .h file. But your comment also made me realize that the implementation also hasn't really made a choice on this; it's just letting the callers do whatever they want in terms of multiple additions of an atom. So we should both comment what invariants this function expects, and then enforce them either in the underlying add_atom function or in a validate function.


//check if a given atom is in the partition
bool contains_atom(AtomBlockId atom_id);

//get the atom_blocks vector
std::vector<AtomBlockId> get_atoms();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This returns a new copy of the std::vector? I assume this not the intention? Better to have this return either const std::vector<AtomBlockId> & or vtr:array_view<const AtomBlockId>?

Method should be const?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.


//set the union of regions for this partition;
void set_part_regions(PartitionRegions pr);
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 add_atoms or set_part_regions imply an array context. However in this case, the array context is a scalar object "PartionRegions". Does it make more sense to rename PartitionRegions to PartitionCollection, which is a non-plural name, and makes clear it is a scalar that contains a collection?

Copy link
Collaborator

@litghost litghost Oct 19, 2020

Choose a reason for hiding this comment

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

To illustrate what I'm talking about, consider atom_blocks, which is plural and is an array-like object. However part_regions is also plural, but is a scalar object.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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();

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 */
40 changes: 40 additions & 0 deletions vpr/src/base/partition_regions.cpp
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
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) {
//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;
}
39 changes: 39 additions & 0 deletions vpr/src/base/partition_regions.h
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:
/**
* Returns the intersection of two PartitionRegions vectors that are passed to it.
*/
PartitionRegions get_intersection(PartitionRegions region);
Copy link
Collaborator

Choose a reason for hiding this comment

The 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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 t_bb that encompasses the vector of regions to provide a quick no if the query it outside any of the supplied regions.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 */
110 changes: 110 additions & 0 deletions vpr/src/base/region.cpp
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;

Region::Region() {
sub_tile = NO_SUBTILE;
}

int Region::get_xmin() {
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) {
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to intersect, should probably make this
bool do_regions_intersection (Region a, Region b);
and it would go outside the class, at the bottom of this header.
If it's accessing private variables, then you'll need to make it a friend function.

bool intersect = true;

int x_min_1 = region.get_xmin();
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 intersection (Region a, Region b);
// May have to be a friend function of class Region.

Region intersect;

int x_min_1 = region.get_xmin();
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()) {
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;
}
48 changes: 48 additions & 0 deletions vpr/src/base/region.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#ifndef REGION_H
#define REGION_H

#include "globals.h"

/**
* @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;

class Region {
public:
Region();
Copy link
Contributor

Choose a reason for hiding this comment

The 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();
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
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 */
19 changes: 19 additions & 0 deletions vpr/src/base/vpr_constraints.cpp
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);
return part_id;
}

void VprConstraints::add_partition(Partition part) {
partitions.push_back(part);
}

vtr::vector<PartitionId, Partition> VprConstraints::get_partitions() {
return partitions;
}
Loading