-
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 13 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,47 @@ | ||
/*************************************** | ||
* Methods for Object RRGraphBuilderView | ||
***************************************/ | ||
#include "vtr_assert.h" | ||
#include "rr_graph_builder_view.h" | ||
|
||
/**************************** | ||
* Constructors | ||
****************************/ | ||
RRGraphBuilderView::RRGraphBuilderView(t_rr_graph_storage* node_storage, | ||
t_rr_node_indices* rr_node_indices) { | ||
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. Always use constructor initializer list for simple assigning of things wherelver possible. The constructor body should only really be used if we have some work that needs to be done (which, incidently, we want to avoid as much as possible in constructors, because we don't have a way to fail in a constructor - except exceptions of course, which we of course want to avoid). So
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, and I see, in this case you're actually initializing values from a base class. Never do that, only access the base class through its constructor. So the base-class should have And then here, you initialize it with
... but see also other comments about inheritance. This is probably not what you want anyway. 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. Thanks for the comment. I was looking for something like this and failed to find related tutorials. I have updated the codes. |
||
node_storage_ = node_storage; | ||
rr_node_indices_ = rr_node_indices; | ||
} | ||
|
||
/**************************** | ||
* Mutators | ||
****************************/ | ||
void RRGraphBuilderView::add_node_to_fast_lookup(const RRNodeId& node, | ||
const int& x, | ||
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. Using references for simple integer types is not very useful. Only use references when you have to. (problem is not only that there is a pointer instead of a shorter immediate type to be passed (and thus not only have to transfer 64 bits instead of 32 in this case, but if you invoke it with a number, the compiler has to generate an intermediate object that has an address), but also, that the compiler needs to often generate special code to make sure not to run into aliasing issues, which you don't have if you just copy things). So integral values (int, char, int64_t, size_t, ...) always should be passed by value if you just have it as an in-parameter. Also for things such as 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. Updated the codes |
||
const int& y, | ||
const t_rr_type& type, | ||
const int& ptc, | ||
const 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()); | ||
|
||
if ((size_t(x) >= (*rr_node_indices_)[type].dim_size(0)) | ||
|| (size_t(y) >= (*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,53 @@ | ||
#ifndef _RR_GRAPH_BUILDER_VIEW_ | ||
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. Avoid leading undrescores in the include-guards as they are reserved for c/c++ compiler internals. 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. Updated. |
||
#define _RR_GRAPH_BUILDER_VIEW_ | ||
|
||
#include <exception> | ||
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. why do you include 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. Removed all the unused headers. |
||
|
||
#include "rr_graph_fwd.h" | ||
#include "rr_node_fwd.h" | ||
#include "rr_graph2.h" | ||
#include "vtr_log.h" | ||
#include "vtr_memory.h" | ||
#include "vpr_utils.h" | ||
#include "vtr_strong_id_range.h" | ||
#include "vtr_array_view.h" | ||
#include "rr_graph_storage.h" | ||
#include "rr_graph_view.h" | ||
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. Below, the types IWYU, but not more. 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. Removed all the unused headers. |
||
|
||
/* An builder of routing resource graph which extends | ||
* the read-only frame view RRGraphView | ||
* | ||
* Note that the builder does not own the storage | ||
* It serves a virtual protocol for | ||
* - rr_graph builder, which requires both mutators and accessors | ||
* | ||
* Note: | ||
* - This is the only data structre allowed to modify a routing resource graph | ||
* | ||
*/ | ||
class RRGraphBuilderView : public RRGraphView { | ||
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 inheritance of classes with non-abstract 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. Second that. Inheritance is reaaaally rarely used in real world code as it has all kinds of issues (fragile base class, hard to figure out what is going on in a stack of classes. In the early days of object oriented programming, everyone thought that was a good idea, but, turns out composition is what is mostly reflecting what we want and inheritance is only really useful to describe abstract things and their implementation (meaning: the concept of interface and implementation)). Base classes are typically only done with interfaces; that is when the base class defines an API of sorts, but with all abstract virtual methods; then there are implementations that implement that (this is what Keith said with the comment above: unless we use the concept of implementing an interface, don't use inheritance). |
||
/**************** | ||
* Constructors | ||
****************/ | ||
public: | ||
RRGraphBuilderView(t_rr_graph_storage* node_storage, | ||
t_rr_node_indices* rr_node_indices); | ||
|
||
/**************** | ||
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 the comments such as Instead, focus on comments on each constructor or method. What are the parameters for these, how are these important, what does or provide the method. These are the relevant parts for someone later using the API. 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. The organization was actually suggested when prototyping the data structure RRGraphObject. We want to guide developers quickly to the constructors and accessors, which the developers care most. I am open to discussion still. This is mainly due to the suggested organization is not clear to me yet. We can have more discussion. |
||
* Accessors all come from RRGraph View | ||
****************/ | ||
|
||
/**************** | ||
* Mutators | ||
****************/ | ||
public: | ||
/* Register a node in the fast look-up */ | ||
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. Each of the parameters need to be explained. This is a public interface, so that is what users of this API will look at to know what to do. Use Doxygen-style commenting style, in which the paramter is in quotes.
The name of the function looks like it mixes intent ("add_node()") and implementation detail : This is a good name for a private method name as there you have more of the implementation details that are important. The public interface should only contain names that describe what we expect from something this class should do. 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. I will see what comments to added. |
||
void add_node_to_fast_lookup(const RRNodeId& node, | ||
const int& x, | ||
const int& y, | ||
const t_rr_type& type, | ||
const int& ptc, | ||
const e_side& side); | ||
}; | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
/*************************************** | ||
* Methods for Object RRGraphView | ||
***************************************/ | ||
#include "vtr_assert.h" | ||
#include "rr_graph_view.h" | ||
|
||
/**************************** | ||
* Constructors | ||
****************************/ | ||
RRGraphView::RRGraphView() { | ||
node_storage_ = nullptr; | ||
rr_node_indices_ = nullptr; | ||
} | ||
|
||
/**************************** | ||
* Accessors | ||
****************************/ | ||
t_rr_type RRGraphView::node_type(const RRNodeId& node) const { | ||
return node_storage_->node_type(node); | ||
} | ||
|
||
RRNodeId RRGraphView::find_node(const int& x, | ||
const int& y, | ||
const t_rr_type& type, | ||
const int& ptc, | ||
const 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); | ||
node_side = SIDES[0]; | ||
} | ||
|
||
/* Currently need to swap x and y for CHANX because of chan, seg convention */ | ||
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 */ | ||
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(); | ||
} | ||
|
||
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 RRGraphView::set_internal_data(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. Is this something we want to allow ? It sounds like that is something that we want to have set once. After all, 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. Agree. Removed |
||
t_rr_node_indices* rr_node_indices) { | ||
node_storage_ = node_storage; | ||
rr_node_indices_ = rr_node_indices; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
#ifndef _RR_GRAPH_OVERLAY_ | ||
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. The class you define here is called Also, avoid leading underscores, as any identifiers and in particular macros with leading underscores are reserved for internal use in C/C++. 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. Updated. |
||
#define _RR_GRAPH_OVERLAY_ | ||
|
||
#include <exception> | ||
|
||
#include "rr_graph_fwd.h" | ||
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. (like in the other place, keep the headers sorted. Since this is a new file, we can start right away using best practices here) 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 |
||
#include "rr_node_fwd.h" | ||
#include "rr_graph2.h" | ||
#include "vtr_log.h" | ||
#include "vtr_memory.h" | ||
#include "vpr_utils.h" | ||
#include "vtr_strong_id_range.h" | ||
#include "vtr_array_view.h" | ||
#include "rr_graph_storage.h" | ||
|
||
/* An read-only routing resource graph | ||
* which is an unified object including pointors to | ||
* - node storage | ||
* - edge_storage | ||
* - node_ptc_storage | ||
* - 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(); | ||
|
||
/**************** | ||
* 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(const RRNodeId& node) const; | ||
tangxifan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/* Returns the index of the specified routing resource node. (x,y) are | ||
* the location within the FPGA, rr_type specifies the type of resource, | ||
* 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 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 | ||
* 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 | ||
*/ | ||
RRNodeId find_node(const int& x, | ||
const int& y, | ||
const t_rr_type& type, | ||
const int& ptc, | ||
const e_side& side = NUM_SIDES) const; | ||
|
||
/**************** | ||
* Mutators | ||
****************/ | ||
public: | ||
/* The ONLY mutators allowed */ | ||
void set_internal_data(t_rr_graph_storage* node_storage, | ||
t_rr_node_indices* rr_node_indices); | ||
|
||
/**************** | ||
* internal data storage, can be accessed by parent class RRGraphBuilderView | ||
****************/ | ||
protected: | ||
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.
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. Changed to private now. |
||
/* 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. Strive for making members that are only set once in the constructor,
That way, it is immediately clear for someone reading the code that these values will never be assigned to again, and it helps make it easier to reason about the code. this also means, that you will have to have a
While the following won't, as it attempts to assign things after the initializer list:
So using this technique of making as much 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. Updated. |
||
/* 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.