-
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 31 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,58 @@ | ||
#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 accidental copy because it could be an expensive operation considering that the | ||
* memory footprint of the data structure could ~ Gb | ||
* Using the following syntax, we prohibit accidental 'pass-by-value' which can be immediately caught | ||
* by compiler | ||
*/ | ||
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,70 @@ | ||
#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 accidental copy because it could be an expensive operation considering that the | ||
* memory footprint of the data structure could ~ Gb | ||
* Using the following syntax, we prohibit accidental 'pass-by-value' which can be immediately caught | ||
* by compiler | ||
*/ | ||
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,108 @@ | ||
#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 | ||
* - For the node which are input/outputs of a grid, there must be a specific side for them. | ||
* Because they should have a specific pin location on the perimeter of a grid. | ||
* - For other types of nodes, there is no need to define a side. However, a default value | ||
* is needed when store the node in the fast look-up data structure. | ||
* Here we just arbitrary use the first side of the SIDE vector as the default value. | ||
* We may consider to use NUM_SIDES as the default value but it will cause an increase | ||
* in the dimension of the fast look-up data structure. | ||
* Please note that in the add_node function, we should keep the SAME convention! | ||
*/ | ||
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_SAFE(type != IPIN && type != OPIN); | ||
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. |
||
} | ||
|
||
/* Pre-check: the x, y, side and ptc should be non negative numbers! Otherwise, return an invalid id */ | ||
if ((0 > x) || (0 > y) || (NUM_SIDES == node_side) || (0 > ptc)) { | ||
return RRNodeId::INVALID(); | ||
} | ||
|
||
/* 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(node); | ||
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)); | ||
} |
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.