-
Notifications
You must be signed in to change notification settings - Fork 415
RR Graph Overlay Creation and Deploy new node_type() API of Overlay #1693
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 28 commits
6a2eb50
cb55acc
584f7fc
b86c734
e927bb5
b25adaa
59b766c
0d697c0
ea1c017
4c82389
dcdb036
387eba7
6e1427b
409e527
32225c8
6ae1016
161d66d
7c0169c
a758762
0ef68b2
abb5041
77f632e
0410c26
baad504
3b5aab5
706a965
0297ce9
4916c1b
4a8443b
2fdabbd
e137b6d
49e3339
76b6f00
82da2e0
ffb4bca
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,15 @@ | ||
#include "rr_graph_builder.h" | ||
|
||
RRGraphBuilder::RRGraphBuilder(t_rr_graph_storage* node_storage, | ||
RRSpatialLookup* node_lookup) | ||
: node_storage_(*node_storage) | ||
, node_lookup_(*node_lookup) { | ||
} | ||
|
||
t_rr_graph_storage& RRGraphBuilder::node_storage() { | ||
return node_storage_; | ||
} | ||
|
||
RRSpatialLookup& RRGraphBuilder::node_lookup() { | ||
return node_lookup_; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
#ifndef RR_GRAPH_BUILDER_H | ||
#define RR_GRAPH_BUILDER_H | ||
|
||
#include "rr_graph_storage.h" | ||
#include "rr_spatial_lookup.h" | ||
|
||
/* A data structure allows data modification on a routing resource graph | ||
* | ||
* Note that the builder does not own the storage | ||
* It serves a virtual protocol for | ||
* - node_storage: store the node list | ||
* - node_lookup: store a fast look-up for the nodes | ||
* | ||
* Note: | ||
* - This is the only data structre allowed to modify a routing resource graph | ||
* | ||
*/ | ||
class RRGraphBuilder { | ||
/* -- Constructors -- */ | ||
public: | ||
/* See detailed comments about the data structures in the internal data storage section of this file */ | ||
RRGraphBuilder(t_rr_graph_storage* node_storage, | ||
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. Might be useful to add a comment about what the parameters are (even if it changes in the future, but as reference for the reader). 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. Yes. Since we will comment each private data structure, I suggest just simply redirect developers to the comments in the internal data storage section of the header file. |
||
RRSpatialLookup* node_lookup); | ||
|
||
tangxifan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/* Disable copy constructors and copy assignment operator | ||
* This is to avoid any duplication of the object | ||
* as it is only interface allowed to modify routing resource graph | ||
*/ | ||
RRGraphBuilder(const RRGraphBuilder&) = delete; | ||
void operator=(const RRGraphBuilder&) = delete; | ||
|
||
/* -- Mutators -- */ | ||
public: | ||
/* Return a writable object for rr_nodes */ | ||
t_rr_graph_storage& node_storage(); | ||
/* Return a writable object for update the fast look-up of rr_node */ | ||
RRSpatialLookup& node_lookup(); | ||
|
||
/* -- Internal data storage -- */ | ||
private: | ||
/* TODO: When the refactoring effort finishes, | ||
* the builder data structure will be the owner of the data storages. | ||
* That is why the reference to storage/lookup is used here. | ||
* It can avoid a lot of code changes once the refactoring is finished | ||
* (there is no function get data directly through the node_storage in DeviceContext). | ||
* If pointers are used, it may cause many codes in client functions | ||
* or inside the data structures to be changed later. | ||
* That explains why the reference is used here temporarily | ||
*/ | ||
/* node-level storage including edge storages */ | ||
t_rr_graph_storage& node_storage_; | ||
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. Only use references if they are const. If you have a modifiable object, use pointers instead (essentially like you did before). Using non-const references creates a lot of confusion, in particular if used as instance variables. 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 fully agree this convention. This rule can improve code readability and help developers to know what they are doing easily. However, as you see in the figure I showed (copied to this PR as well), the builder function will be the owner of the data storages. That is why I temporarily use the reference here. It can avoid a lot of code changes once the refactoring is finished (there is no function get data directly through the Here are my concerns and I am open to discussion. 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. OK with me. Eventually the ability to get the node_storage will be removed (best) or if necessary re-written to use pointers. |
||
/* Fast look-up for rr nodes */ | ||
RRSpatialLookup& node_lookup_; | ||
}; | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
#include "rr_graph_view.h" | ||
|
||
RRGraphView::RRGraphView(const t_rr_graph_storage& node_storage, | ||
const RRSpatialLookup& node_lookup) | ||
: node_storage_(node_storage) | ||
, node_lookup_(node_lookup) { | ||
} | ||
|
||
t_rr_type RRGraphView::node_type(RRNodeId node) const { | ||
return node_storage_.node_type(node); | ||
} | ||
|
||
const RRSpatialLookup& RRGraphView::node_lookup() const { | ||
return node_lookup_; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
#ifndef RR_GRAPH_VIEW_H | ||
#define RR_GRAPH_VIEW_H | ||
|
||
#include "rr_graph_storage.h" | ||
#include "rr_spatial_lookup.h" | ||
|
||
/* An read-only routing resource graph | ||
* which is an unified object including pointors to | ||
* - node storage | ||
* - TODO: edge_storage | ||
* - TODO: node_ptc_storage | ||
* - TODO: node_fan_in_storage | ||
* - rr_node_indices | ||
* | ||
* Note that the RRGraphView does not own the storage | ||
* It serves a virtual read-only protocol for | ||
* - placer | ||
* - router | ||
* - timing analyzer | ||
* - GUI | ||
* | ||
* Note that each client of rr_graph may get a frame view of the object | ||
* The RRGraphView is the complete frame view of the routing resource graph | ||
* - This helps to reduce the memory footprint for each client | ||
* - This avoids massive changes for each client on using the APIs | ||
* as each frame view provides adhoc APIs for each client | ||
* | ||
* TODO: more compact frame views will be created, e.g., | ||
* - a mini frame view: contains only node and edges, representing the connectivity of the graph | ||
* - a geometry frame view: an extended mini frame view with node-level attributes, | ||
* in particular geometry information (type, x, y etc). | ||
* | ||
*/ | ||
class RRGraphView { | ||
/* -- Constructors -- */ | ||
public: | ||
/* See detailed comments about the data structures in the internal data storage section of this file */ | ||
RRGraphView(const t_rr_graph_storage& node_storage, | ||
const RRSpatialLookup& node_lookup); | ||
|
||
/* Disable copy constructors and copy assignment operator | ||
* This is to avoid any duplication of the object | ||
* as it is only interface allowed to access routing resource graph | ||
*/ | ||
RRGraphView(const RRGraphView&) = delete; | ||
void operator=(const RRGraphView&) = delete; | ||
|
||
/* -- Accessors -- */ | ||
/* TODO: The accessors may be turned into private later if they are replacable by 'questionin' | ||
* kind of accessors | ||
*/ | ||
public: | ||
/* Get the type of a routing resource node */ | ||
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. Probably good to add a TODO comment saying that the goal is to have these accessors private at some point totally replaced with the 'questioin' kind of methods. 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. Yes. The TODO comment is useful. But not every API will be converted to the |
||
t_rr_type node_type(RRNodeId node) const; | ||
|
||
/* Return the fast look-up data structure for queries from client functions */ | ||
const RRSpatialLookup& node_lookup() const; | ||
|
||
/* -- Internal data storage -- */ | ||
/* Note: only read-only object or data structures are allowed!!! */ | ||
private: | ||
/* node-level storage including edge storages */ | ||
const t_rr_graph_storage& node_storage_; | ||
/* Fast look-up for rr nodes */ | ||
const RRSpatialLookup& node_lookup_; | ||
}; | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
#include "vtr_assert.h" | ||
#include "rr_spatial_lookup.h" | ||
|
||
RRSpatialLookup::RRSpatialLookup(t_rr_node_indices& rr_node_indices) | ||
: rr_node_indices_(rr_node_indices) { | ||
} | ||
|
||
RRNodeId RRSpatialLookup::find_node(int x, | ||
int y, | ||
t_rr_type type, | ||
int ptc, | ||
e_side side) const { | ||
/* Find actual side to be used */ | ||
e_side node_side = side; | ||
if (type == IPIN || type == OPIN) { | ||
VTR_ASSERT_MSG(side != NUM_SIDES, "IPIN/OPIN must specify desired side (can not be default NUM_SIDES)"); | ||
} else { | ||
VTR_ASSERT(type != IPIN && type != OPIN); | ||
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. Don't really need this assert, as the prior if guarantees it is true. 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. The assertation is just a safety check. Currently, I prefer to changing it to VTR_ASSERT_SAFE, which will not be considered unless in debug mode (no runtime cost in release mode). I am also o.k. to remove it. Please let me know what you think. 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. OK, your call. I'm OK with VTR_ASSERT_SAFE if you prefer, although I personally don't put asserts right in an if-chain where it is obvious the assert can't fire based on the if/else condition. |
||
node_side = SIDES[0]; | ||
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. Comment why this is done. Node side not relevant for nodes that aren't IPINs or OPINS; set to an arbitrary side? 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. Agree. Added comments to clarify this. |
||
} | ||
|
||
/* Currently need to swap x and y for CHANX because of chan, seg convention | ||
* This is due to that the fast look-up builders uses (y, x) coordinate when | ||
* registering a CHANX node in the look-up | ||
* TODO: Once the builders is reworked for use consistent (x, y) convention, | ||
* the following swapping can be removed | ||
*/ | ||
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. Agreed. I did this years ago to allow re-use of the same data structure; it is ugly! |
||
size_t node_x = x; | ||
size_t node_y = y; | ||
if (CHANX == type) { | ||
std::swap(node_x, node_y); | ||
} | ||
|
||
VTR_ASSERT_SAFE(3 == rr_node_indices_[type].ndims()); | ||
|
||
/* Sanity check to ensure the x, y, side and ptc are in range | ||
* - Return an valid id by searching in look-up when all the parameters are in range | ||
* - Return an invalid id if any out-of-range is detected | ||
*/ | ||
if (size_t(type) >= rr_node_indices_.size()) { | ||
return RRNodeId::INVALID(); | ||
} | ||
|
||
if (node_x >= rr_node_indices_[type].dim_size(0)) { | ||
return RRNodeId::INVALID(); | ||
} | ||
|
||
if (node_y >= rr_node_indices_[type].dim_size(1)) { | ||
return RRNodeId::INVALID(); | ||
} | ||
|
||
if (node_side >= rr_node_indices_[type].dim_size(2)) { | ||
return RRNodeId::INVALID(); | ||
} | ||
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. Should add the same code to return RRNodeID::INVALID for node_x, node_y, node_side, ptc being < 0 too. 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. Agree. However, the 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. Ah, I didn't think of that. In that case I'm OK with just a comment noting that will happen. If you've added the pre-check for < 0 already that's fine too. |
||
|
||
if (size_t(ptc) >= rr_node_indices_[type][node_x][node_y][node_side].size()) { | ||
return RRNodeId::INVALID(); | ||
} | ||
|
||
return RRNodeId(rr_node_indices_[type][node_x][node_y][node_side][ptc]); | ||
} | ||
|
||
void RRSpatialLookup::add_node(RRNodeId node, | ||
int x, | ||
int y, | ||
t_rr_type type, | ||
int ptc, | ||
e_side side) { | ||
VTR_ASSERT_SAFE(3 == rr_node_indices_[type].ndims()); | ||
|
||
/* Expand the fast look-up if the new node is out-of-range | ||
* This may seldom happen because the rr_graph building function | ||
* should ensure the fast look-up well organized | ||
*/ | ||
VTR_ASSERT(type < rr_node_indices_.size()); | ||
VTR_ASSERT(0 <= x); | ||
VTR_ASSERT(0 <= y); | ||
|
||
if ((x >= int(rr_node_indices_[type].dim_size(0))) | ||
|| (y >= int(rr_node_indices_[type].dim_size(1))) | ||
|| (size_t(side) >= rr_node_indices_[type].dim_size(2))) { | ||
rr_node_indices_[type].resize({std::max(rr_node_indices_[type].dim_size(0), size_t(x) + 1), | ||
std::max(rr_node_indices_[type].dim_size(1), size_t(y) + 1), | ||
std::max(rr_node_indices_[type].dim_size(2), size_t(side) + 1)}); | ||
} | ||
|
||
if (size_t(ptc) >= rr_node_indices_[type][x][y][side].size()) { | ||
rr_node_indices_[type][x][y][side].resize(ptc + 1); | ||
} | ||
|
||
/* Resize on demand finished; Register the node */ | ||
rr_node_indices_[type][x][y][side][ptc] = int(size_t(node)); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
#ifndef RR_SPATIAL_LOOKUP_H | ||
#define RR_SPATIAL_LOOKUP_H | ||
|
||
#include "vpr_types.h" | ||
|
||
/******************************************************************** | ||
* A data structure storing the fast look-up for the nodes | ||
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'd explain what the lookup is rather than saying it is a fast lookup. We want to find the id of an rr_node given information about its physical position and type; usually this is needed during rr-graph building. 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. Indeed. Reworked the comments. |
||
* in a routing resource graph | ||
* | ||
* The data structure allows users to | ||
* - Update the look-up with new nodes | ||
* - Find the id of a node with given information, e.g., x, y, type etc. | ||
********************************************************************/ | ||
class RRSpatialLookup { | ||
/* -- Constructors -- */ | ||
public: | ||
/* Explicitly define the only way to create an object */ | ||
explicit RRSpatialLookup(t_rr_node_indices& rr_node_indices); | ||
|
||
/* Disable copy constructors and copy assignment operator | ||
* This is to avoid any duplication of the object | ||
* as it is only interface allowed to access node look-up of a routing resource graph | ||
*/ | ||
RRSpatialLookup(const RRSpatialLookup&) = delete; | ||
void operator=(const RRSpatialLookup&) = delete; | ||
|
||
/* -- Accessors -- */ | ||
public: | ||
/* Returns the index of the specified routing resource node. (x,y) are | ||
* the location within the FPGA, rr_type specifies the type of resource, | ||
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. Minor edits I suggest. and ptc gives the number of this resource -> and ptc gives a unique number for resources of that type (e.g. CHANX) at that (x,y). 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. Comment applied. |
||
* and ptc gives the number of this resource. ptc is the class number, | ||
* pin number or track number, depending on what type of resource this | ||
* is. All ptcs start at 0 and go up to pins_per_clb-1 or the equivalent. | ||
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. All ptcs start at 0 and are positive; for example for CHANX resources they would normally go from 0 to channel_width - 1 at that (x,y), and for OPINs or IPINS they would normally start at 0 and go to the number of pins on a block at that (x,y) location. 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. Agree. I have reworked the comments to provide more insights on the ptc number depending on the |
||
* There are class_inf size SOURCEs + SINKs, type->num_pins IPINs + OPINs, | ||
* and max_chan_width CHANX and CHANY (each). | ||
* | ||
* Note that for segments (CHANX and CHANY) of length > 1, the segment is | ||
* given an rr_index based on the (x,y) location at which it starts (i.e. | ||
* lowest (x,y) location at which this segment exists). | ||
* This routine also performs error checking to make sure the node 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'd move this comment "This routine also " to the end of the comment block. 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. Comment applied. |
||
* question exists. | ||
* | ||
* The 'side' argument only applies to IPIN/OPIN types, and specifies which | ||
* side of the grid tile the node should be located on. The value is ignored | ||
* for non-IPIN/OPIN types | ||
*/ | ||
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. Should mention that RR_NODE_INVALID is returned if the node does not exist. 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. Agree. Comment applied. 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 the comment is in the wrong spot (add_node, but should be in find_node). |
||
RRNodeId find_node(int x, | ||
int y, | ||
t_rr_type type, | ||
int ptc, | ||
e_side side = NUM_SIDES) const; | ||
|
||
/* -- Mutators -- */ | ||
public: | ||
/* Register a node in the fast look-up | ||
* - You must have a node id | ||
* 1. a valid one is to register a node in the lookup | ||
* 2. an invalid id means the removal of a node from the lookup | ||
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. "2. an invalid id means the removal of a node from the lookup" I don't think the .cpp file code actually does that, and it would be a strange convention. I think this part of the comment should be removed. If the comment is correct, I think we should change the convention and instead make a remove_node function. Or perhaps this comment is intended for the data storage itself and means we can store an invalid node ID in rr_node_indices to indicate that node does not exist. That would be OK, but that comment should go with the storage declaration, not in front of the add_node function. 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. You are right. The use of invalid id is very tricky. It just unregisters a node from the lookup. |
||
* - (x, y) are the coordinate of the node to be indexable in the fast look-up | ||
* - type is the type of a node | ||
* - ptc is a feature number of a node, which can be | ||
* 1. pin index in a tile when type is OPIN/IPIN | ||
* 2. track index in a routing channel when type is CHANX/CHANY | ||
* - side is the side of node on the tile, applicable to OPIN/IPIN | ||
* | ||
* Note that the add node here will not create a node in the node list | ||
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. Note that a node added with this call will not create a node in the rr_graph_storage node list .. 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. Comment applied. |
||
* You MUST add the node in the t_rr_node_storage so that the node is valid | ||
* | ||
* TODO: Consider to try to return a reference to *this so that we can do chain calls | ||
* - .add_node(...) | ||
* - .add_node(...) | ||
* - .add_node(...) | ||
* As such, multiple node addition could be efficiently implemented | ||
*/ | ||
void add_node(RRNodeId node, | ||
tangxifan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
int x, | ||
int y, | ||
t_rr_type type, | ||
int ptc, | ||
e_side side); | ||
|
||
/* -- Internal data storage -- */ | ||
private: | ||
/* TODO: When the refactoring effort finishes, | ||
* the data structure will be the owner of the data storages. | ||
* That is why the reference is used here. | ||
* It can avoid a lot of code changes once the refactoring is finished | ||
* (there is no function get data directly through the rr_node_indices in DeviceContext). | ||
* If pointers are used, it may cause many codes in client functions | ||
* or inside the data structures to be changed later. | ||
* That explains why the reference is used here temporarily | ||
*/ | ||
/* Fast look-up */ | ||
t_rr_node_indices& rr_node_indices_; | ||
}; | ||
|
||
#endif |
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.
I'd suggest to always sort all headers alphabetically.
That way it is harder possible to create merge conflicts if everyone does the same.
(I realize that this is not how the practice was in this file, but I suggest to adopt that practice. It also sometimes helps finding hidden dependencies in which header files only work when included in a particular sequence)
So in emacs that would be: mark the block with all includes then
M-x sort-lines
; other editors have a similar feature.In general it is adivsable to follow the practices like in this style guide: grouping blocks of c-headers, c++ headers and project headers.
(Might be worth-while to have a separate cleanup-run of this, not necessarily in this pull request. So consider this an FYI with future influence)
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.
It makes sense to me. I will avoid modifying the
vpr_context.h
because it is not the focus of this PR. But I will force the alphabetical order of header files in newly created files in this PR.