Created draft of new constraints class header file #1535

sfkhalid · 2020-09-11T14:54:34Z

A draft header file containing an overview of the new constraints class was created. The constraints class will read in constraints information from an XML file. This information will be used during the packing and placement stages of VPR

Motivation and Context

This class is being created to address issue #932 Add Placement Constraints to VPR

vaughnbetz · 2020-09-11T15:13:14Z

vpr/src/base/vpr_constraints.h

@@ -0,0 +1,93 @@
+#ifndef VPR_SRC_BASE_VPR_CONSTRAINTS_H_


To be more consistent with other code, shorten the include guard to VPR_CONSTRAINTS_H

vpr/src/base/vpr_constraints.h

sfkhalid · 2020-09-17T14:57:23Z

Sample XML 1:

<vpr_constraints>

<partition_list>
    <!-- A partition consists of a unique identifier, and a set of
         atom primitives.

         Note: in the limit each atom could be in a separate partition -->
    <partition name="partition_name_or_id">
        <!-- Adds all the atoms whose name match the regex to the enclosing partition -->
        <add_atoms name_pattern="my_primitive1"/> <!-- name_pattern can be an explicit name -->
        <add_atoms name_pattern="my_processor/alu32/*/"> <!-- name pattern could also be a regex pattern -->
    </partition>
</partition_list>

<placement_constraints>
    <!-- Force a particular partition to be placed within a specific region -->
    <placement_region partition_pattern="regex" || atom_pattern="regex"> <!-- partition_pattern may be wild-carded and could match multiple partitions -->
        <rect xlow="int" ylow="int" subtilelow="int" xhigh="int" yhigh="int" subtilehigh="int">
    </placement_region>
</placement_constraints>

</vpr_constraints>

sfkhalid · 2020-09-17T14:58:02Z

Sample XML 1:

<vpr_constraints>

<partition_list>
    <!-- A partition consists of a unique identifier, and a set of
         atom primitives.

         Note: in the limit each atom could be in a separate partition -->
    <partition name="partition_name_or_id">
        <!-- Adds all the atoms whose name match the regex to the enclosing partition -->
        <add_atoms name_pattern="my_primitive1"/> <!-- name_pattern can be an explicit name -->
        <add_atoms name_pattern="my_processor/alu32/*/"> <!-- name pattern could also be a regex pattern -->
    </partition>
</partition_list>

<placement_constraints>
    <!-- Force a particular partition to be placed within a specific region -->
    <placement_region partition_pattern="regex" || atom_pattern="regex"> <!-- partition_pattern may be wild-carded and could match multiple partitions -->
        <rect xlow="int" ylow="int" subtilelow="int" xhigh="int" yhigh="int" subtilehigh="int">
    </placement_region>
</placement_constraints>

</vpr_constraints>

Based on Kevin's draft XML in issue 932 with minor changes

sfkhalid · 2020-09-17T15:01:08Z

Sample XML 2:

<vpr_constraints>

<partition_list>
    <!-- A partition consists of a unique identifier, and a set of
         atom primitives.

         Note: in the limit each atom could be in a separate partition -->
    <partition name="partition_name_or_id">
        <!-- Adds all the atoms whose name match the regex to the enclosing partition -->
        <add_atoms name_pattern="my_primitive1"/> <!-- name_pattern can be an explicit name -->
        <add_atoms name_pattern="my_processor/alu32/*/"> <!-- name pattern could also be a regex pattern -->
    </partition>
</partition_list>

<region_list>
	<!-- A region is defined by its x, y, and subtile bounds. Could consider including z bounds for future use (with 3D FPGA designs) -->
	<region name="region_name_or_id">
        <rect xlow="int" ylow="int" subtilelow="int" xhigh="int" yhigh="int" subtilehigh="int">
    </region>
<region_list>

<placement_constraints>
    <!-- Force a particular partition to be placed within a specific region -->
    <constraint> 
        <add_partition partition_pattern="partition_1">
		<add_region region_pattern="region_1">
    </constraint>
</placement_constraints>

</vpr_constraints>

vpr/src/base/partition.h

vpr/src/base/partition_regions.h

vpr/src/base/region.h

vpr/src/base/vpr_constraints.h

vaughnbetz · 2020-09-17T17:05:36Z

XML: I think XML 2 has too many distinct concepts that are given Ids (don't think individual regions need names/ids) but others may disagree so you should propose this.
Think there should be an XML 0 option too where the PartitionRegion is inside the Partition definition. That would match your current class description.

These are reasonable options, so showing both in a design review is reasonable.

sfkhalid · 2020-09-21T14:01:20Z

<vpr_constraints>

<partition_list>
    <!-- A partition consists of a unique identifier, and a set of
         atom primitives.

         Note: in the limit each atom could be in a separate partition -->
    <partition name="partition_name_or_id">
        <!-- Adds all the atoms whose name match the regex to the enclosing partition -->
        <add_atoms name_pattern="my_primitive1"/> <!-- name_pattern can be an explicit name -->
        <add_atoms name_pattern="my_processor/alu32/*/"> <!-- name pattern could also be a regex pattern -->
		<!-- Add region that partition should be placed in -->
		<add_region xlow="int" ylow="int" subtilelow="int" xhigh="int" yhigh="int" subtilehigh="int"/>
    </partition>
</partition_list>

</vpr_constraints>

XML_0 option where the PartitionRegion is inside the Partition definition.

vaughnbetz · 2020-09-21T18:04:29Z

I would remove the indenting of the regions; they are directly underneath the partition hierarchy, just like add_atoms.

sfkhalid · 2020-09-24T14:13:35Z

XML_0 with indenting fixed
`<vpr_constraints>

<partition_list>
    <!-- A partition consists of a unique identifier, a set of
         atom primitives, and regions it can be placed in.

         Note: in the limit each atom could be in a separate partition -->
    <partition name="partition_name_or_id">
        <!-- Adds all the atoms whose name match the regex to the enclosing partition -->
        <add_atoms name_pattern="my_primitive1"/> <!-- name_pattern can be an explicit name -->
        <add_atoms name_pattern="my_processor/alu32/*/"/> <!-- name pattern could also be a regex pattern -->
        <!-- Add region that partition should be placed in -->
        <add_region xlow="int" ylow="int" subtilelow="int" xhigh="int" yhigh="int" subtilehigh="int"/>
    </partition>
</partition_list>

</vpr_constraints>`

vpr/src/base/partition.h

sfkhalid · 2020-09-24T15:53:08Z

vpr/src/base/partition.h

+    PartitionRegions get_partition_regions();
+
+  private:
+    PartitionId id;


comment this too - unique id for this partition

sfkhalid · 2020-09-24T15:55:21Z

vpr/src/base/partition.h

+
+  private:
+    PartitionId id;
+    std::string name;                     //name of the partition


name will be unique across partitions

HackerFoo · 2020-10-16T18:44:04Z

vpr/src/base/partition.cpp

+}
+
+void Partition::set_partition_id(PartitionId _part_id) {
+    id = _part_id;


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.

HackerFoo · 2020-10-16T19:51:02Z

@sfkhalid Here's an example of how to get a std::vector index from a pointer to one of the elements: https://gist.github.com/HackerFoo/3e1ccd5872a1149c54330942a88a595d

sfkhalid · 2020-10-16T20:16:13Z

@sfkhalid Here's an example of how to get a std::vector index from a pointer to one of the elements: https://gist.github.com/HackerFoo/3e1ccd5872a1149c54330942a88a595d

Thanks, I'll take a look at this.

vaughnbetz · 2020-10-17T02:06:31Z

Thanks Dusty. I guess there's no alternative to pointer arithmetic if we need this. I was hoping the standard library wrapped a nice function around things like this (like std::distance, but it only works for iterators which won't quite do what we need -- http://www.cplusplus.com/reference/iterator/distance/).
Sarah, you should also consider if you really need to return a partition id from a partition. If callers ask for the partition id for an atom, they will get an id that way and then pass that into the a get_partition function by id or some such function. But they won't need to ask for an id given a partition object; they'll be doing the opposite pattern of access. So I suggest not coding the get_partition_id function until you see you have callers that need it.

mithro · 2020-10-18T19:53:10Z

@litghost - Could you also take a look at this?

mithro · 2020-10-18T19:53:38Z

@andrewb1999 -- Could you take a look at this and see if it is interesting / helps at all with the work you are doing at UPenn?

vpr/src/base/partition.h

vpr/src/base/partition_regions.cpp

vpr/src/base/region.cpp

vpr/src/base/region.h

vpr/src/base/vpr_constraints.cpp

vpr/src/base/vpr_constraints.h

vaughnbetz · 2020-10-15T17:11:16Z

vpr/src/base/vpr_constraints.h

+    void add_partition(Partition part);
+
+    //Method to return the partitions vector
+    vtr::vector<PartitionId, Partition> get_partitions();


Should return the partition that corresponds to a certain ID, not the whole vector.
Then you need to think about how you quickly get that partition from a partitionID that is passed in. If the index in the vector is the partition id, then you can simply look it up. But then you need to delete the partition_id you are storing or make sure it matches the index. Safer to delete it, and compute partition_id using nicer C++ pointer arithmetic (done in some rr_graph functions now); web search should find it.

litghost · 2020-10-19T16:35:20Z

vpr/src/base/partition.h

+    void set_name(std::string _part_name);
+
+    //add an id to the atom_blocks vector
+    void add_to_atoms(AtomBlockId atom_id);


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?

Question: Is there a use case for providing a range of atoms, versus one at a time?

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.

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

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?

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.

litghost · 2020-10-19T16:37:30Z

vpr/src/base/partition.h

+    bool contains_atom(AtomBlockId atom_id);
+
+    //get the atom_blocks vector
+    std::vector<AtomBlockId> get_atoms();


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?

Good point. Yes, should be const, and should just return a reference. Haven't used array_view; may be more appropriate still.

vpr/src/base/partition.h

litghost · 2020-10-19T16:39:59Z

vpr/src/base/partition.h

+    std::vector<AtomBlockId> get_atoms();
+
+    //set the union of regions for this partition;
+    void set_part_regions(PartitionRegions pr);


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?

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.

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.

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.

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.

litghost · 2020-10-19T16:42:04Z

vpr/src/base/partition_regions.h

+    /**
+     * Returns the intersection of two PartitionRegions vectors that are passed to it.
+     */
+    PartitionRegions get_intersection(PartitionRegions region);


Consider making this a static method that consumes two arguments?

litghost · 2020-10-19T16:45:26Z

vpr/src/base/partition_regions.h

+    std::vector<Region> get_partition_regions();
+
+  private:
+    std::vector<Region> partition_regions; //vector of regions that a partition can be placed in


I'm not sure a vector of Region is the right data structure here. I feel like you want a spatial tree, e.g. :

https://en.wikipedia.org/wiki/Binary_space_partitioning

https://en.wikipedia.org/wiki/K-d_tree

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.

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.

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?

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.

litghost

Comments in

HackerFoo · 2020-10-19T17:53:55Z

@vaughnbetz Yeah, I saw std::distance but it seemed more complicated for O(1) access. I've used the method I gave in C and it is defined behavior as long as the pointer is into the vector. One drawback is that the elements should be aligned to a power of 2 for the fastest computation of the index, but this is probably not an issue here, and there is a possibility that storing indices could be slower than division anyway.

If you don't want to promote pointer arithmetic scattered across the codebase, this could be wrapped in a vtr::vector method e.g. std::optional<K> vector<K, V>::getKey(const& V).

… region functions accordingly

…ction

… these and changed a class name to PartitionRegion

libs/libvtrutil/src/vtr_geometry.h

vpr/src/base/region.cpp

sfkhalid · 2020-10-27T15:18:01Z

@vaughnbetz This PR should be ready to merge now - made changes to the Partition and VprConstraints classes as we discussed, changed the name of PartitionRegions -> PartitionRegion, and made the comments for the classes doxygen compatible.

vaughnbetz

Some final clean up / consistency suggestions. You can do those in a new PR.

vaughnbetz · 2020-10-29T15:15:25Z

vpr/src/base/partition.h

+
+class Partition {
+  public:
+    const std::string get_name();


Put a brief comment for doxygen on top of these functions too.

vaughnbetz · 2020-10-29T15:20:02Z

vpr/src/base/partition_region.h

+     *
+     *   @param part_region     The PartitionRegion that the calling object will be intersected with
+     */
+    PartitionRegion get_intersection(PartitionRegion part_region);


Lets follow Keith's suggestion and be more like Rect.
Move this to the bottom of the header file (outside the class), and make it
PartitionRegion insersection (PartitionRegion a, PartitionRegion b);

vaughnbetz · 2020-10-29T15:21:07Z

vpr/src/base/region.cpp

+Region::Region() {
+    sub_tile = NO_SUBTILE;
+
+    //default rect for a region is (-1, -1, -1, -1)


Explain why (invalid, to help catch uninitialized use).

vaughnbetz · 2020-10-29T15:24:04Z

vpr/src/base/region.cpp

+    sub_tile = _sub_tile;
+}
+
+bool Region::do_regions_intersect(Region region) {


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.

vaughnbetz · 2020-10-29T15:29:22Z

vpr/src/base/region.cpp

+    return intersect;
+}
+
+Region Region::regions_intersection(Region region) {


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.

vaughnbetz · 2020-10-29T15:37:34Z

vpr/test/test_vpr_constraints.cpp

+}
+
+TEST_CASE("PartitionRegion", "[vpr]") {
+    Region r1;


Add comment on what this does.

vaughnbetz · 2020-10-29T15:38:06Z

vpr/test/test_vpr_constraints.cpp

+}
+
+TEST_CASE("Partition", "[vpr]") {
+    Partition part;


Comment what it tests.

vaughnbetz · 2020-10-29T15:38:46Z

vpr/test/test_vpr_constraints.cpp

+}
+
+TEST_CASE("VprConstraints", "[vpr]") {
+    PartitionId part_id(0);


Comment what it tests.

vaughnbetz · 2020-10-29T15:39:29Z

vpr/test/test_vpr_constraints.cpp

+    REQUIRE(regions[1].get_region_rect() == int_rect_2);
+}
+
+TEST_CASE("PartRegionIntersect2", "[vpr]") {


Comment what it tests.

vaughnbetz · 2020-10-29T15:40:19Z

vpr/test/test_vpr_constraints.cpp

+}
+
+TEST_CASE("RegionLocked", "[vpr]") {
+    Region r1;


Comment what it tests.

Created draft of new constraints class header file

42492c7

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

vaughnbetz requested changes Sep 11, 2020

View reviewed changes

Added new classes - Partition, Region, PartitionRegions

31b826e