Skip to content

Deploy RRGraphBuilder in RRGraph Clock Builder to replace the use of rr_node_indices #1801

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.

Sign up for GitHub

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 7 commits into from
Jul 18, 2021
Merged

Deploy RRGraphBuilder in RRGraph Clock Builder to replace the use of rr_node_indices #1801

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c37643c
[VPR] Deploy RRGraphBuilder to clock network builder
tangxifan Jul 14, 2021
e6e2a2f
[VPR] Deploy RRSpatialLookup to clock connection builder
tangxifan Jul 14, 2021
8804edd
[VPR] Delete unused functions
tangxifan Jul 14, 2021
3dfafbf
[VPR] Code format fix
tangxifan Jul 14, 2021
1f3c410
[VPR] Fix compiler warning
tangxifan Jul 14, 2021
d4a76f6
[VPR] Remove the hard requirement on side option when adding a node; …
tangxifan Jul 17, 2021
ab9dc4e
[VPR] Reworked RRSpatialLookup: added a private API find_nodes() to a…
tangxifan Jul 17, 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
45 changes: 45 additions & 0 deletions vpr/src/device/rr_spatial_lookup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,51 @@ std::vector<RRNodeId> RRSpatialLookup::find_channel_nodes(int x,
return channel_nodes;
}

std::vector<RRNodeId> RRSpatialLookup::find_sink_nodes(int x,
int y) const {
/* TODO: The implementation of this API should be worked
* when rr_node_indices adapts RRNodeId natively!
*/
std::vector<RRNodeId> sink_nodes;

/* Pre-check: the x, y, type are valid! Otherwise, return an empty vector */
if (x < 0 || y < 0) {
return sink_nodes;
}

VTR_ASSERT_SAFE(3 == rr_node_indices_[SINK].ndims());

/* Sanity check to ensure the x, y, side are in range
* - Return a list of valid ids by searching in look-up when all the parameters are in range
* - Return an empty list if any out-of-range is detected
*/
if (size_t(SINK) >= rr_node_indices_.size()) {
return sink_nodes;
}

if (size_t(x) >= rr_node_indices_[SINK].dim_size(0)) {
return sink_nodes;
}

if (size_t(y) >= rr_node_indices_[SINK].dim_size(1)) {
return sink_nodes;
}

/* By default, we always added the channel nodes to the TOP side (to save memory) */
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment looks wrong (talking about channels).

This code also points out why we should rework the underlying rr_node_indices implementation; it's strange to have a multi-dimensional array matrix where some dimensions must always be accessed with a specific index. Hopefully that is on your near-term agenda Xifan. Could put a TODO in to refactor that way, if you think such a comment would be a useful reminder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the typo. Just fixed it.
I think it will be a good time to rethink the rr_node_indices data structure once all the related APIs are replaced.
Actually, it is not too far. I expect only we need another 2 pull requests, before fully shadowing the current rr_node_indices.

I put a TODO to remind us during the refactoring. I will take a deep look into the rr_node_indices data structure and propose something.

e_side node_side = TOP;
if (node_side >= rr_node_indices_[SINK].dim_size(2)) {
return sink_nodes;
}

for (const auto& node : rr_node_indices_[SINK][x][y][node_side]) {
if (RRNodeId(node)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could comment this to say you're inserting only the valid ids in the returned vector.

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 the comment.

sink_nodes.push_back(RRNodeId(node));
}
}

return sink_nodes;
}

std::vector<RRNodeId> RRSpatialLookup::find_nodes_at_all_sides(int x,
int y,
t_rr_type rr_type,
Expand Down
12 changes: 12 additions & 0 deletions vpr/src/device/rr_spatial_lookup.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,18 @@ class RRSpatialLookup {
int y,
t_rr_type type) const;

/**
* Returns the indices of the specified routing resource nodes,
* representing virtual sinks.
* - (x, y) are the coordinate of the sink nodes within the FPGA
*
* Note:
* - Return an empty list if there are no sinks at the given (x, y) location
* - The node list returned only contain valid ids
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: contain -> contains

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out. Fixed.

*/
std::vector<RRNodeId> find_sink_nodes(int x,
int y) const;

/**
* Like find_node() but returns all matching nodes on all the sides.
* This is particularly useful for getting all instances
Expand Down
67 changes: 33 additions & 34 deletions vpr/src/route/clock_connection_builders.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ void RoutingToClockConnection::create_switches(const ClockRRGraphBuilder& clock_
std::srand(seed);

auto& device_ctx = g_vpr_ctx.device();
auto& rr_node_indices = device_ctx.rr_node_indices;
const auto& node_lookup = device_ctx.rr_graph.node_lookup();

int virtual_clock_network_root_idx = create_virtual_clock_network_sink_node(switch_location.x, switch_location.y);
{
Expand All @@ -61,10 +61,8 @@ void RoutingToClockConnection::create_switches(const ClockRRGraphBuilder& clock_
}

// rr_node indices for x and y channel routing wires and clock wires to connect to
auto x_wire_indices = get_rr_node_chan_wires_at_location(
rr_node_indices, CHANX, switch_location.x, switch_location.y);
auto y_wire_indices = get_rr_node_chan_wires_at_location(
rr_node_indices, CHANY, switch_location.x, switch_location.y);
auto x_wire_indices = node_lookup.find_channel_nodes(switch_location.x, switch_location.y, CHANX);
auto y_wire_indices = node_lookup.find_channel_nodes(switch_location.x, switch_location.y, CHANY);
auto clock_indices = clock_graph.get_rr_node_indices_at_switch_location(
clock_to_connect_to, switch_point_name, switch_location.x, switch_location.y);

Expand All @@ -76,13 +74,13 @@ void RoutingToClockConnection::create_switches(const ClockRRGraphBuilder& clock_
// Connect to x-channel wires
unsigned num_wires_x = x_wire_indices.size() * fc;
for (size_t i = 0; i < num_wires_x; i++) {
clock_graph.add_edge(rr_edges_to_create, x_wire_indices[i], clock_index, arch_switch_idx);
clock_graph.add_edge(rr_edges_to_create, size_t(x_wire_indices[i]), clock_index, arch_switch_idx);
}

// Connect to y-channel wires
unsigned num_wires_y = y_wire_indices.size() * fc;
for (size_t i = 0; i < num_wires_y; i++) {
clock_graph.add_edge(rr_edges_to_create, y_wire_indices[i], clock_index, arch_switch_idx);
clock_graph.add_edge(rr_edges_to_create, size_t(y_wire_indices[i]), clock_index, arch_switch_idx);
}

// Connect to virtual clock sink node
Expand All @@ -95,35 +93,38 @@ int RoutingToClockConnection::create_virtual_clock_network_sink_node(
int x,
int y) {
auto& device_ctx = g_vpr_ctx.mutable_device();
auto& rr_nodes = device_ctx.rr_nodes;
rr_nodes.emplace_back();
auto node_index = rr_nodes.size() - 1;
auto& rr_graph = device_ctx.rr_nodes;
auto& node_lookup = device_ctx.rr_graph_builder.node_lookup();
rr_graph.emplace_back();
RRNodeId node_index = RRNodeId(rr_graph.size() - 1);

//Determine the a valid PTC
std::vector<int> nodes_at_loc;
get_rr_node_indices(device_ctx.rr_node_indices,
x, y,
SINK,
&nodes_at_loc);
std::vector<RRNodeId> nodes_at_loc = node_lookup.find_sink_nodes(x, y);

int max_ptc = 0;
for (int inode : nodes_at_loc) {
max_ptc = std::max<int>(max_ptc, device_ctx.rr_nodes[inode].ptc_num());
for (RRNodeId inode : nodes_at_loc) {
max_ptc = std::max<int>(max_ptc, rr_graph.node_ptc_num(inode));
}
int ptc = max_ptc + 1;

rr_nodes[node_index].set_ptc_num(ptc);
rr_nodes[node_index].set_coordinates(x, y, x, y);
rr_nodes[node_index].set_capacity(1);
rr_nodes[node_index].set_cost_index(SINK_COST_INDEX);
rr_nodes[node_index].set_type(SINK);
rr_graph.set_node_ptc_num(node_index, ptc);
rr_graph.set_node_coordinates(node_index, x, y, x, y);
rr_graph.set_node_capacity(node_index, 1);
rr_graph.set_node_cost_index(node_index, SINK_COST_INDEX);
rr_graph.set_node_type(node_index, SINK);
float R = 0.;
float C = 0.;
rr_nodes[node_index].set_rc_index(find_create_rr_rc_data(R, C));
rr_graph.set_node_rc_index(node_index, find_create_rr_rc_data(R, C));

add_to_rr_node_indices(device_ctx.rr_node_indices, rr_nodes, node_index);
// Use a generic way when adding nodes to lookup.
// However, since the SINK node has the same xhigh/xlow as well as yhigh/ylow, we can probably use a shortcut
for (int ix = rr_graph.node_xlow(node_index); ix <= rr_graph.node_xhigh(node_index); ++ix) {
for (int iy = rr_graph.node_ylow(node_index); iy <= rr_graph.node_yhigh(node_index); ++iy) {
node_lookup.add_node(node_index, ix, iy, rr_graph.node_type(node_index), rr_graph.node_ptc_num(node_index), 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.

Looks like we use SIDES[0] to insert the sink node, and look for it later on the TOP side. Hence should we use TOP consistently or SIDES[0] consistently to make sure this is always in alignment? I assume SIDES[0] == TOP so this all works, but it still seems dangerous as if anyone ever edits the order of SIDES we'd subtly break code written this way.

Copy link
Contributor Author

@tangxifan tangxifan Jul 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Agree. We should not expose the side argument to developers because they may feel confused.
You are right on the SIDE[0] which is actually the TOP side. This convention has been there for a while (I do not know who introduced it). I am o.k. to change it if necessary.
Indeed, developers may mess up.
For example, they added a channel node to the BOTTOM side but in find_node(), it will always search the TOP side.

vtr-verilog-to-routing/vpr/src/device/rr_spatial_lookup.cpp

Lines 23 to 29 in c06800f

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];
}

However, I think it is not a good idea to force the developers to give a dummy side when addethe API. I modified the API so that the side option will have a default value (SIDE[0]) if not specified. In client functions, I remove all the usage of the SIDE[0] for non-IPIN/OPIN nodes.

}
}

return node_index;
return size_t(node_index);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change the return type of this function to RRNodeId and remove the cast? (May have to change the calling function too some then).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked the dependency of this API. It is used only by another API. It is simple change. Done.

}

/*
Expand Down Expand Up @@ -243,7 +244,7 @@ size_t ClockToPinsConnection::estimate_additional_nodes() {

void ClockToPinsConnection::create_switches(const ClockRRGraphBuilder& clock_graph, t_rr_edge_info_set* rr_edges_to_create) {
auto& device_ctx = g_vpr_ctx.device();
auto& rr_node_indices = device_ctx.rr_node_indices;
const auto& node_lookup = device_ctx.rr_graph.node_lookup();
auto& grid = clock_graph.grid();

for (size_t x = 0; x < grid.width(); x++) {
Expand Down Expand Up @@ -304,13 +305,11 @@ void ClockToPinsConnection::create_switches(const ClockRRGraphBuilder& clock_gra
clock_y_offset = -1; // pick the chanx below the block
}

auto clock_pin_node_idx = get_rr_node_index(
rr_node_indices,
x,
y,
IPIN,
clock_pin_idx,
side);
auto clock_pin_node_idx = node_lookup.find_node(x,
y,
IPIN,
clock_pin_idx,
side);

auto clock_network_indices = clock_graph.get_rr_node_indices_at_switch_location(
clock_to_connect_from,
Expand All @@ -320,7 +319,7 @@ void ClockToPinsConnection::create_switches(const ClockRRGraphBuilder& clock_gra

//Create edges depending on Fc
for (size_t i = 0; i < clock_network_indices.size() * fc; i++) {
clock_graph.add_edge(rr_edges_to_create, clock_network_indices[i], clock_pin_node_idx, arch_switch_idx);
clock_graph.add_edge(rr_edges_to_create, clock_network_indices[i], size_t(clock_pin_node_idx), arch_switch_idx);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change the function prototype to avoid the cast (send in an RRNodeId instead)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my reply in a previous comment on the t_rr_edge_info

}
}
}
Expand Down
47 changes: 31 additions & 16 deletions vpr/src/route/clock_network_builders.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ void ClockNetwork::set_num_instance(int num_inst) {

void ClockNetwork::create_rr_nodes_for_clock_network_wires(ClockRRGraphBuilder& clock_graph,
t_rr_graph_storage* rr_nodes,
t_rr_node_indices* rr_node_indices,
RRGraphBuilder& rr_graph_builder,
t_rr_edge_info_set* rr_edges_to_create,
int num_segments) {
for (int inst_num = 0; inst_num < get_num_inst(); inst_num++) {
create_rr_nodes_and_internal_edges_for_one_instance(clock_graph, rr_nodes, rr_node_indices, rr_edges_to_create, num_segments);
create_rr_nodes_and_internal_edges_for_one_instance(clock_graph, rr_nodes, rr_graph_builder, rr_edges_to_create, num_segments);
}
}

Expand Down Expand Up @@ -217,7 +217,7 @@ size_t ClockRib::estimate_additional_nodes(const DeviceGrid& grid) {

void ClockRib::create_rr_nodes_and_internal_edges_for_one_instance(ClockRRGraphBuilder& clock_graph,
t_rr_graph_storage* rr_nodes,
t_rr_node_indices* rr_node_indices,
RRGraphBuilder& rr_graph_builder,
t_rr_edge_info_set* rr_edges_to_create,
int num_segments) {
// Only chany wires need to know the number of segments inorder
Expand Down Expand Up @@ -275,7 +275,7 @@ void ClockRib::create_rr_nodes_and_internal_edges_for_one_instance(ClockRRGraphB
ptc_num,
Direction::BIDIR,
rr_nodes,
rr_node_indices);
rr_graph_builder);
clock_graph.add_switch_location(get_name(), drive.name, drive_x, y, drive_node_idx);

// create rib wire to the right and left of the drive point
Expand All @@ -285,14 +285,14 @@ void ClockRib::create_rr_nodes_and_internal_edges_for_one_instance(ClockRRGraphB
ptc_num,
Direction::DEC,
rr_nodes,
rr_node_indices);
rr_graph_builder);
auto right_node_idx = create_chanx_wire(drive_x + 1,
x_end,
y,
ptc_num,
Direction::INC,
rr_nodes,
rr_node_indices);
rr_graph_builder);
record_tap_locations(x_start + x_offset,
x_end,
y,
Expand All @@ -313,7 +313,7 @@ int ClockRib::create_chanx_wire(int x_start,
int ptc_num,
Direction direction,
t_rr_graph_storage* rr_nodes,
t_rr_node_indices* rr_node_indices) {
RRGraphBuilder& rr_graph_builder) {
rr_nodes->emplace_back();
auto node_index = rr_nodes->size() - 1;
auto node = rr_nodes->back();
Expand Down Expand Up @@ -343,7 +343,15 @@ int ClockRib::create_chanx_wire(int x_start,
}
node.set_cost_index(CHANX_COST_INDEX_START + seg_index); // Actual value set later

add_to_rr_node_indices(*rr_node_indices, *rr_nodes, node_index);
/* Add the node to spatial lookup */
auto& rr_graph = (*rr_nodes);
RRNodeId chanx_node = RRNodeId(node_index);
for (int ix = rr_graph.node_xlow(chanx_node); ix <= rr_graph.node_xhigh(chanx_node); ++ix) {
for (int iy = rr_graph.node_ylow(chanx_node); iy <= rr_graph.node_yhigh(chanx_node); ++iy) {
//TODO: CHANX uses odd swapped x/y indices here. Will rework once rr_node_indices is shadowed
rr_graph_builder.node_lookup().add_node(chanx_node, iy, ix, rr_graph.node_type(chanx_node), rr_graph.node_ptc_num(chanx_node), SIDES[0]);
}
}

return node_index;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider changing return type to RRNodeId, and directly making node_index an RRNodeId (and therefore wouldn't need chanx_node).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I believe, we can use the API add_node_to_all_locs() as you suggested in another PR #1800. These for loops will be replaced by a simple line:

rr_graph_builder.add_node_to_all_locs(chanx_node)

I leave a TODO comment here to remind me to finish the API replacement after PR #1800 is merged.
Let me know if this is fine for you.

}
Expand Down Expand Up @@ -511,7 +519,7 @@ size_t ClockSpine::estimate_additional_nodes(const DeviceGrid& grid) {

void ClockSpine::create_rr_nodes_and_internal_edges_for_one_instance(ClockRRGraphBuilder& clock_graph,
t_rr_graph_storage* rr_nodes,
t_rr_node_indices* rr_node_indices,
RRGraphBuilder& rr_graph_builder,
t_rr_edge_info_set* rr_edges_to_create,
int num_segments) {
auto& grid = clock_graph.grid();
Expand Down Expand Up @@ -565,7 +573,7 @@ void ClockSpine::create_rr_nodes_and_internal_edges_for_one_instance(ClockRRGrap
ptc_num,
Direction::BIDIR,
rr_nodes,
rr_node_indices,
rr_graph_builder,
num_segments);
clock_graph.add_switch_location(get_name(), drive.name, x, drive_y, drive_node_idx);

Expand All @@ -576,15 +584,15 @@ void ClockSpine::create_rr_nodes_and_internal_edges_for_one_instance(ClockRRGrap
ptc_num,
Direction::DEC,
rr_nodes,
rr_node_indices,
rr_graph_builder,
num_segments);
auto right_node_idx = create_chany_wire(drive_y + 1,
y_end,
x,
ptc_num,
Direction::INC,
rr_nodes,
rr_node_indices,
rr_graph_builder,
num_segments);

// Keep a record of the rr_node idx that we will use to connects switches to at
Expand All @@ -609,7 +617,7 @@ int ClockSpine::create_chany_wire(int y_start,
int ptc_num,
Direction direction,
t_rr_graph_storage* rr_nodes,
t_rr_node_indices* rr_node_indices,
RRGraphBuilder& rr_graph_builder,
int num_segments) {
rr_nodes->emplace_back();
auto node_index = rr_nodes->size() - 1;
Expand Down Expand Up @@ -640,7 +648,14 @@ int ClockSpine::create_chany_wire(int y_start,
}
node.set_cost_index(CHANX_COST_INDEX_START + num_segments + seg_index);

add_to_rr_node_indices(*rr_node_indices, *rr_nodes, node_index);
/* Add the node to spatial lookup */
auto& rr_graph = (*rr_nodes);
RRNodeId chany_node = RRNodeId(node_index);
for (int ix = rr_graph.node_xlow(chany_node); ix <= rr_graph.node_xhigh(chany_node); ++ix) {
for (int iy = rr_graph.node_ylow(chany_node); iy <= rr_graph.node_yhigh(chany_node); ++iy) {
rr_graph_builder.node_lookup().add_node(chany_node, ix, iy, rr_graph.node_type(chany_node), rr_graph.node_ptc_num(chany_node), SIDES[0]);
}
}

return node_index;
}
Expand Down Expand Up @@ -678,14 +693,14 @@ size_t ClockHTree::estimate_additional_nodes(const DeviceGrid& /*grid*/) {

void ClockHTree::create_rr_nodes_and_internal_edges_for_one_instance(ClockRRGraphBuilder& clock_graph,
t_rr_graph_storage* rr_nodes,
t_rr_node_indices* rr_node_indices,
RRGraphBuilder& rr_graph_builder,
t_rr_edge_info_set* rr_edges_to_create,
int num_segments) {
//Remove unused parameter warning
(void)clock_graph;
(void)num_segments;
(void)rr_nodes;
(void)rr_node_indices;
(void)rr_graph_builder;
(void)rr_edges_to_create;

VPR_FATAL_ERROR(VPR_ERROR_ROUTE, "HTrees are not yet supported.\n");
Expand Down
Loading