Skip to content

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

Merged
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
6a2eb50
[VPR] Add an overlay RRGraph object; Different from t_rr_graph_view, …
tangxifan Mar 24, 2021
cb55acc
[VPR] Update rr_graph overlay to enable its deployment in DeviceContext
tangxifan Mar 24, 2021
584f7fc
[VPR] Replace node type() API with overlay API in check_rr_graph.cpp
tangxifan Mar 24, 2021
b86c734
Merge branch 'master' of https://github.com/verilog-to-routing/vtr-ve…
tangxifan Mar 24, 2021
e927bb5
[VPR] add overlay initialization for rr_graph loading from external f…
tangxifan Mar 24, 2021
b25adaa
[VPR] Code format fix
tangxifan Mar 24, 2021
59b766c
[VPR] Code format fix
tangxifan Mar 24, 2021
0d697c0
[VPR] Add rr graph overlay to rr_graph reader and writer
tangxifan Mar 24, 2021
ea1c017
[VPR] Add a prototype of RRGraphView (read-only) and its builder exte…
tangxifan Mar 26, 2021
4c82389
[VPR] Bug fix in RRGraphView API and deploy to placer delay matrix co…
tangxifan Mar 26, 2021
dcdb036
[VPR] Code format fix
tangxifan Mar 26, 2021
387eba7
[VPR] Deploy mutator to rr_node_indice builder as an example
tangxifan Mar 26, 2021
6e1427b
Merge branch 'master' into rr_graph_node_type_overlay
tangxifan Apr 1, 2021
409e527
Merge branch 'master' into rr_graph_node_type_overlay
tangxifan Apr 7, 2021
32225c8
[VPR] Reworked rr_graph_view and rr_graph_builder; Created rr_spatial…
tangxifan Apr 7, 2021
6ae1016
[VPR] code format fix; add comments to API
tangxifan Apr 8, 2021
161d66d
Merge branch 'master' into rr_graph_node_type_overlay
tangxifan Apr 8, 2021
7c0169c
[VPR] Add comments to explain why references are currently used in RR…
tangxifan Apr 9, 2021
a758762
Merge branch 'rr_graph_node_type_overlay' of https://github.com/tangx…
tangxifan Apr 9, 2021
0ef68b2
Merge branch 'master' into rr_graph_node_type_overlay
tangxifan Apr 13, 2021
abb5041
Merge branch 'master' into rr_graph_node_type_overlay
tangxifan Apr 14, 2021
77f632e
[VPR] Add protection to RRGraphBuilder, RRGraphView and RRSpatial to …
tangxifan Apr 15, 2021
0410c26
[VPR] Code format fix
tangxifan Apr 15, 2021
baad504
[VPR] Compiler warning fix
tangxifan Apr 15, 2021
3b5aab5
[VPR] Add RRGraphBuilder to DeviceContext; Identify a critical limita…
tangxifan Apr 15, 2021
706a965
[VPR] Reworked comments to emphasize on confusing codes while delete …
tangxifan Apr 15, 2021
0297ce9
[VPR] Reworked the comment about avoid copy contructors for RRGraphBu…
tangxifan Apr 16, 2021
4916c1b
Merge branch 'master' into rr_graph_node_type_overlay
tangxifan Apr 19, 2021
4a8443b
Merge branch 'master' into rr_graph_node_type_overlay
tangxifan May 20, 2021
2fdabbd
Merge branch 'rr_graph_node_type_overlay' of https://github.com/tangx…
tangxifan May 21, 2021
e137b6d
[VPR] Update comments to avoid confusion and clarify
tangxifan May 21, 2021
49e3339
[VPR] Patch comments and remove the inproper assert (which should be …
tangxifan May 21, 2021
76b6f00
Merge branch 'master' into rr_graph_node_type_overlay
vaughnbetz May 25, 2021
82da2e0
Merge branch 'master' into rr_graph_node_type_overlay
vaughnbetz May 25, 2021
ffb4bca
Merge branch 'master' into rr_graph_node_type_overlay
vaughnbetz May 26, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion vpr/src/base/vpr_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
#include "vtr_vector.h"
#include "atom_netlist.h"
#include "clustered_netlist.h"
#include "rr_graph_view.h"
Copy link
Contributor

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)

Copy link
Contributor Author

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.

#include "rr_graph_storage.h"
#include "rr_graph_builder.h"
#include "rr_node.h"
#include "rr_rc_data.h"
#include "tatum/TimingGraph.hpp"
Expand Down Expand Up @@ -145,7 +147,6 @@ struct DeviceContext : public Context {
/*
* Structures to define the routing architecture of the FPGA.
*/

t_rr_graph_storage rr_nodes; // autogenerated in build_rr_graph

std::vector<t_rr_indexed_data> rr_indexed_data; // [0 .. num_rr_indexed_data-1]
Expand All @@ -162,6 +163,21 @@ struct DeviceContext : public Context {
///@brief The indicies of rr nodes of a given type at a specific x,y grid location
t_rr_node_indices rr_node_indices; // [0..NUM_RR_TYPES-1][0..grid.width()-1][0..grid.width()-1][0..size-1]

/* TODO: remove this interface from device_context once the code refactoring is completed
* because it should be part of the rr_graph view
*/
RRSpatialLookup rr_spatial_lookup{rr_node_indices};

/* A read-only view of routing resource graph to be the ONLY database
* for client functions: GUI, placer, router, timing analyzer etc.
*/
RRGraphView rr_graph{rr_nodes, rr_spatial_lookup};

/* A writeable view of routing resource graph to be the ONLY database
* for routing resource graph builder functions.
*/
RRGraphBuilder rr_graph_builder{&rr_nodes, &rr_spatial_lookup};

///@brief Autogenerated in build_rr_graph based on switch fan-in. [0..(num_rr_switches-1)]
std::vector<t_rr_switch_inf> rr_switch_inf;

Expand Down
15 changes: 15 additions & 0 deletions vpr/src/device/rr_graph_builder.cpp
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_;
}
56 changes: 56 additions & 0 deletions vpr/src/device/rr_graph_builder.h
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,
Copy link
Contributor

Choose a reason for hiding this comment

The 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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 node_storage in DeviceContext). My concern is that if I use pointers, it may cause many codes in client functions or inside the data structures to be changed later.

Here are my concerns and I am open to discussion.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
15 changes: 15 additions & 0 deletions vpr/src/device/rr_graph_view.cpp
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_;
}
68 changes: 68 additions & 0 deletions vpr/src/device/rr_graph_view.h
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 */
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 questionin methods. But the questionin style is preferred.

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
93 changes: 93 additions & 0 deletions vpr/src/device/rr_spatial_lookup.cpp
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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];
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
*/
Copy link
Contributor

Choose a reason for hiding this comment

The 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();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. However, the node_x, node_y are already in the size_t data type, they cannot be less than 0 (converted from x , y which are int). If x is negative, when converted to size_t, it will be a huge number and out-of-range in the fast look-up. But it is really tricky. So, I added a pre-check on the x, y, node_side and ptc to return invalid ids when one of them is negative.

Copy link
Contributor

Choose a reason for hiding this comment

The 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));
}
97 changes: 97 additions & 0 deletions vpr/src/device/rr_spatial_lookup.h
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor edits I suggest.
the location within -> the grid location within

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 node_type.

* 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Comment applied.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Actually, I should not state it because we should not promote such use of the API.
Therefore, I removed this line. We should add an API remove_node(). I put this TODO item in the comment.

* - (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
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
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
Loading