-
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 25 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,24 @@ | ||
/*************************************** | ||
* Methods for Object RRGraphBuilder | ||
***************************************/ | ||
#include "rr_graph_builder.h" | ||
|
||
/**************************** | ||
* Constructors | ||
****************************/ | ||
RRGraphBuilder::RRGraphBuilder(t_rr_graph_storage* node_storage, | ||
RRSpatialLookup* node_lookup) | ||
: node_storage_(*node_storage) | ||
, node_lookup_(*node_lookup) { | ||
} | ||
|
||
/**************************** | ||
* Mutators | ||
****************************/ | ||
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,63 @@ | ||
#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 | ||
****************/ | ||
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. ... Still not a fan of these massive comments. They don't really serve a purpose (as for a well-structured class it is immediately clear where is what), so they are mostly just counter the goal to reduce vertical whitespace. We have only so much vertical space on our monitors, so we should try to maximize information. If it helps readability I sometimes do that with a single line comment that looks a bit emphasized, maybe with dashes:
Sounds like these large comments were decided before me, but I'd strongly encourage to re-evaluate :) 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. These dummy lines should be removed. |
||
public: | ||
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); | ||
|
||
/* Disable copy constructors | ||
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 looks like a lot of comment for something fairly common. 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 left the comment about the copy constructor is because it is the first time I used these C++ syntax. 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. No worries, leave it as you feel comfortable. As for the additional explanation
I'd probably emphasize that we want to avoid an accidental copy not because of the interface situation, but because (once the builder actually owns the data), it will be a very expensive operation, copying gigabytes of data. If we prevent copying, then accidental 'pass-by-value' are immediately caught by the compiler for instance. 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. No problem. I think it is more accurate. Changed. |
||
* This is to avoid any duplication of the object | ||
* as it is only interface allowed to modify routing resource graph | ||
*/ | ||
RRGraphBuilder(const RRGraphBuilder&) = delete; | ||
|
||
/* Disable copy assignment operator */ | ||
void operator=(const RRGraphBuilder&) = delete; | ||
|
||
tangxifan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/**************** | ||
* 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,25 @@ | ||
/*************************************** | ||
* Methods for Object RRGraphView | ||
***************************************/ | ||
#include "rr_graph_view.h" | ||
|
||
/**************************** | ||
* Constructors | ||
****************************/ | ||
RRGraphView::RRGraphView(const t_rr_graph_storage& node_storage, | ||
const RRSpatialLookup& node_lookup) | ||
: node_storage_(node_storage) | ||
, node_lookup_(node_lookup) { | ||
} | ||
|
||
/**************************** | ||
* Accessors | ||
****************************/ | ||
t_rr_type RRGraphView::node_type(RRNodeId node) const { | ||
return node_storage_.node_type(node); | ||
} | ||
|
||
const RRSpatialLookup& RRGraphView::node_lookup() const { | ||
/* Return a constant object rather than a writable one */ | ||
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. mmh, I am not a fan of comments stating the obvious. Problem with that is of course, that comments become noise instead of actual useful information, so readers of the code might learn to ignore comments even though it might contain something useful. Also, the vertical space used is a lot, so that makes it harder to navigate the code (similar with the massive three-line comments about constructors and accessors which should't really be needed as anyone sees that anyway - but it sounds like this was decided for some other reason ? Still not a fan :) ). 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 you are right. I have not written the comments carefully so that there are some redundant comments. Should remove this line. Also, I would like to share my opinions about writing comments (feel free to bug and share your experience)
As such, developers can focus on reading header files for the key data structure and only look into specific functions in source files when necessary. 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. Fully agree. |
||
return node_lookup_; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
#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: | ||
RRGraphView(const t_rr_graph_storage& node_storage, | ||
const RRSpatialLookup& node_lookup); | ||
|
||
/* Disable copy constructors | ||
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 here: these constructors can just be two lines without comment. |
||
* This is to avoid any duplication of the object | ||
* as it is only interface allowed to access routing resource graph | ||
*/ | ||
RRGraphView(const RRGraphView&) = delete; | ||
|
||
/* Disable copy assignment operator */ | ||
void operator=(const RRGraphView&) = delete; | ||
|
||
/**************** | ||
* 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 a read-only object for performing fast look-up in rr_node */ | ||
const RRSpatialLookup& node_lookup() const; | ||
|
||
/**************** | ||
* internal data storage | ||
****************/ | ||
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 @@ | ||
/*************************************** | ||
* Methods for Object RRSpatialLookup | ||
***************************************/ | ||
#include "vtr_assert.h" | ||
#include "rr_spatial_lookup.h" | ||
|
||
/**************************** | ||
* Constructors | ||
****************************/ | ||
RRSpatialLookup::RRSpatialLookup(t_rr_node_indices& rr_node_indices) | ||
: rr_node_indices_(rr_node_indices) { | ||
} | ||
|
||
/**************************** | ||
* Accessors | ||
****************************/ | ||
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 */ | ||
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 place where I'd expect a more detailed comment, as this is clearly something that can surprise. 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. More explanation has been added. |
||
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 | ||
* Data type of rr_node_indice: | ||
* typedef std::array<vtr::NdMatrix<std::vector<int>, 3>, NUM_RR_TYPES> t_rr_node_indices; | ||
* Note that x, y and side are the 1st, 2nd and 3rd dimensions of the vtr::NdMatrix | ||
* ptc is in the std::vector<int> | ||
*/ | ||
if (size_t(type) >= rr_node_indices_.size()) { | ||
/* Node type is out of range, return an invalid index */ | ||
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 avoid comments that just restate exactly what the code does. They will train readers to ignore comments, which is dangerous. And they use up vertical space, at least 6 lines alone here when counting also the To further reduce vertical space usage, you can consider the this if permitted by this project's readability rules: simple return based on on a condition is more compact (and as readable) with the following:
So, where interesting stuff is happening, we should emphasize it with code and comments, but boring things are nice when compact. 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. I removed all the redundant comments. 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. SGTM as well. |
||
return RRNodeId::INVALID(); | ||
} | ||
|
||
if (node_x >= rr_node_indices_[type].dim_size(0)) { | ||
/* Node x is out of range, return an invalid index */ | ||
return RRNodeId::INVALID(); | ||
} | ||
|
||
if (node_y >= rr_node_indices_[type].dim_size(1)) { | ||
/* Node y is out of range, return an invalid index */ | ||
return RRNodeId::INVALID(); | ||
} | ||
|
||
if (node_side >= rr_node_indices_[type].dim_size(2)) { | ||
/* Node side is out of range, return an invalid index */ | ||
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()) { | ||
/* Ptc is out of range, return an invalid index */ | ||
return RRNodeId::INVALID(); | ||
} | ||
|
||
/* Reaching here, it means that node exists in the look-up, return the id */ | ||
return RRNodeId(rr_node_indices_[type][node_x][node_y][node_side][ptc]); | ||
} | ||
|
||
/**************************** | ||
* Mutators | ||
****************************/ | ||
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)); | ||
} |
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.