Skip to content

Remove legacy data structure rr_node_indices from DeviceContext #1828

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 3 commits into from
Aug 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 4 additions & 15 deletions vpr/src/base/vpr_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -160,26 +160,15 @@ struct DeviceContext : public Context {
///@brief Reverse look-up from RR node to non-configurably connected node set (index into rr_nonconf_node_sets)
std::unordered_map<int, int> rr_node_to_non_config_node_set;

///@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
* TODO: Currently, we use reference pointers to ensure that the rr_spatial_lookup is always
* synchronized with the rr_node_indices but this causes a lot of confusion for developers
* The temporary fix should be patched as soon as possible.
/* A writeable view of routing resource graph to be the ONLY database
* for routing resource graph builder functions.
*/
RRSpatialLookup rr_spatial_lookup{rr_node_indices};
RRGraphBuilder rr_graph_builder{&rr_nodes};

/* 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};
RRGraphView rr_graph{rr_nodes, rr_graph_builder.node_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
10 changes: 6 additions & 4 deletions vpr/src/device/rr_graph_builder.cpp
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
#include "vtr_log.h"
#include "rr_graph_builder.h"

RRGraphBuilder::RRGraphBuilder(t_rr_graph_storage* node_storage,
RRSpatialLookup* node_lookup)
: node_storage_(*node_storage)
, node_lookup_(*node_lookup) {
RRGraphBuilder::RRGraphBuilder(t_rr_graph_storage* node_storage)
: node_storage_(*node_storage) {
}

t_rr_graph_storage& RRGraphBuilder::node_storage() {
Expand Down Expand Up @@ -48,3 +46,7 @@ void RRGraphBuilder::add_node_to_all_locs(RRNodeId node) {
}
}
}

void RRGraphBuilder::clear() {
node_lookup_.clear();
}
8 changes: 5 additions & 3 deletions vpr/src/device/rr_graph_builder.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ 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,
RRSpatialLookup* node_lookup);
RRGraphBuilder(t_rr_graph_storage* node_storage);

/* Disable copy constructors and copy assignment operator
* This is to avoid accidental copy because it could be an expensive operation considering that the
Expand Down Expand Up @@ -49,6 +48,9 @@ class RRGraphBuilder {
*/
void add_node_to_all_locs(RRNodeId node);

/* Clear all the underlying data storage */
void clear();

/* -- Internal data storage -- */
private:
/* TODO: When the refactoring effort finishes,
Expand All @@ -63,7 +65,7 @@ class RRGraphBuilder {
/* node-level storage including edge storages */
t_rr_graph_storage& node_storage_;
/* Fast look-up for rr nodes */
RRSpatialLookup& node_lookup_;
RRSpatialLookup node_lookup_;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have the RRGraphView and RRGraphBuilder APIs, we should make doxygen documentation for them. Best done as a follow-up issue, so I'll file an issue for that (make doxygen-compatible comments, and add to the VPR API on the read-the-docs page).

#endif
3 changes: 1 addition & 2 deletions vpr/src/device/rr_graph_view.h
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
#ifndef RR_GRAPH_VIEW_H
#define RR_GRAPH_VIEW_H

#include "rr_graph_storage.h"
#include "rr_spatial_lookup.h"
#include "rr_graph_builder.h"

/* An read-only routing resource graph
* which is an unified object including pointors to
Expand Down
26 changes: 24 additions & 2 deletions vpr/src/device/rr_spatial_lookup.cpp
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
#include "vtr_assert.h"
#include "rr_spatial_lookup.h"

RRSpatialLookup::RRSpatialLookup(t_rr_node_indices& rr_node_indices)
: rr_node_indices_(rr_node_indices) {
RRSpatialLookup::RRSpatialLookup() {
}

RRNodeId RRSpatialLookup::find_node(int x,
Expand Down Expand Up @@ -273,3 +272,26 @@ void RRSpatialLookup::resize_nodes(int x,
std::max(rr_node_indices_[type].dim_size(2), size_t(side) + 1)});
}
}

void RRSpatialLookup::reorder(const vtr::vector<RRNodeId, RRNodeId> dest_order) {
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 expect some sort of unit-test for this method ?
(I know it is moved from some other place without change, so ok not adding a test for now; but now, it is actually a nicely testable functionality)

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. This feature is already under testing in one of the test case in vtr_reg_basic:

script_params_list_add = --reorder_rr_graph_nodes_algorithm random_shuffle

I am not sure if this can be categorized as a unit-test. If not, we should think about it once we can extract the RRGraphBuilder and RRGraphView as an independent library.
Let me know what you think. I think unit test is a good idea.

// update rr_node_indices, a map to optimize rr_index lookups
for (auto& grid : rr_node_indices_) {
for (size_t x = 0; x < grid.dim_size(0); x++) {
for (size_t y = 0; y < grid.dim_size(1); y++) {
for (size_t s = 0; s < grid.dim_size(2); s++) {
for (auto& node : grid[x][y][s]) {
if (node != OPEN) {
node = size_t(dest_order[RRNodeId(node)]);
}
}
}
}
}
}
}

void RRSpatialLookup::clear() {
for (auto& data : rr_node_indices_) {
data.clear();
}
}
20 changes: 9 additions & 11 deletions vpr/src/device/rr_spatial_lookup.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#define RR_SPATIAL_LOOKUP_H

#include "vtr_geometry.h"
#include "vtr_vector.h"
#include "vpr_types.h"

/********************************************************************
Expand All @@ -17,7 +18,7 @@ class RRSpatialLookup {
/* -- Constructors -- */
public:
/* Explicitly define the only way to create an object */
explicit RRSpatialLookup(t_rr_node_indices& rr_node_indices);
explicit RRSpatialLookup();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we don't take a parameter anymore, the explicit is not needed anymore.
The explicit is only needed to tell the compiler to not consider this constructor in an automatic type-conversion, but if there are zero parameters, that is not an issue anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense to me. Will remove it.


/* Disable copy constructors and copy assignment operator
* This is to avoid accidental copy because it could be an expensive operation considering that the
Expand Down Expand Up @@ -185,6 +186,12 @@ class RRSpatialLookup {
t_rr_type type,
e_side side);

/* Reorder the internal look up to be more memory efficient */
void reorder(const vtr::vector<RRNodeId, RRNodeId> dest_order);

/* Clear all the data inside */
void clear();

/* -- Internal data queries -- */
private:
/* An internal API to find all the nodes in a specific location with a given type
Expand All @@ -199,17 +206,8 @@ class RRSpatialLookup {

/* -- 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: TODO: Should rework the data type. Currently it is based on a 3-dimensional arrqay mater where some dimensions must always be accessed with a specific index. Such limitation should be overcome */
Copy link
Contributor

Choose a reason for hiding this comment

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

typo to fix: 3-dimensional array matrix

t_rr_node_indices& rr_node_indices_;
t_rr_node_indices rr_node_indices_;
};

#endif
9 changes: 4 additions & 5 deletions vpr/src/route/rr_graph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ void create_rr_graph(const t_graph_type graph_type,

process_non_config_sets();

verify_rr_node_indices(grid, device_ctx.rr_node_indices, device_ctx.rr_nodes);
verify_rr_node_indices(grid, device_ctx.rr_graph, device_ctx.rr_nodes);

print_rr_graph_stats();

Expand Down Expand Up @@ -1342,12 +1342,10 @@ void free_rr_graph() {

device_ctx.read_rr_graph_filename.clear();

for (auto& data : device_ctx.rr_node_indices) {
data.clear();
}

device_ctx.rr_nodes.clear();

device_ctx.rr_graph_builder.clear();

device_ctx.rr_indexed_data.clear();

device_ctx.rr_switch_inf.clear();
Expand Down Expand Up @@ -2423,6 +2421,7 @@ static vtr::NdMatrix<std::vector<int>, 4> alloc_and_load_track_to_pin_lookup(vtr
return track_to_pin_lookup;
}

/* TODO: This function should adapt RRNodeId */
std::string describe_rr_node(int inode) {
auto& device_ctx = g_vpr_ctx.device();
const auto& rr_graph = device_ctx.rr_graph;
Expand Down
Loading