Skip to content

RR Edge ID Verification #3164

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
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
2 changes: 2 additions & 0 deletions vpr/src/base/SetupVPR.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,8 @@ static void SetupRouterOpts(const t_options& Options, t_router_opts* RouterOpts)
RouterOpts->has_choke_point = Options.router_opt_choke_points;
RouterOpts->custom_3d_sb_fanin_fanout = Options.custom_3d_sb_fanin_fanout;
RouterOpts->with_timing_analysis = Options.timing_analysis;

RouterOpts->verify_rr_switch_id = Options.verify_rr_switch_id;
}

static void SetupAnnealSched(const t_options& Options,
Expand Down
10 changes: 10 additions & 0 deletions vpr/src/base/read_options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1637,6 +1637,16 @@ argparse::ArgumentParser create_arg_parser(const std::string& prog_name, t_optio
.default_value("on")
.show_in(argparse::ShowIn::HELP_ONLY);

gen_grp.add_argument<bool, ParseOnOff>(args.verify_rr_switch_id, "--verify_rr_switch_id")
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a routing option, add it to route_grp

.help(
"Verify that the switch IDs in the routing file are consistent with those in the RR Graph."
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a space or '\n' at the end of each line.

Copy link
Contributor

Choose a reason for hiding this comment

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

document this new option in the rst file

"Set this to false when switch IDs in the routing file may differ from the RR Graph."
"For example, when analyzing different timing corners using the same netlist, placement, and routing files,"
"the RR switch IDs in the RR Graph may differ due to changes in delays."
"In such cases, set this option to false so that the switch IDs from the RR Graph are used, and those in the routing file are ignored.")
.default_value("on")
.show_in(argparse::ShowIn::HELP_ONLY);

gen_grp.add_argument(args.target_device_utilization, "--target_utilization")
.help(
"Sets the target device utilization."
Expand Down
1 change: 1 addition & 0 deletions vpr/src/base/read_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ struct t_options {
argparse::ArgValue<e_timing_update_type> timing_update_type;
argparse::ArgValue<bool> CreateEchoFile;
argparse::ArgValue<bool> verify_file_digests;
argparse::ArgValue<bool> verify_rr_switch_id;
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 this option is only applicable to the routing stage and its variable should move there

argparse::ArgValue<std::string> device_layout;
argparse::ArgValue<float> target_device_utilization;
argparse::ArgValue<e_constant_net_method> constant_net_method;
Expand Down
103 changes: 93 additions & 10 deletions vpr/src/base/read_route.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,32 @@
#include "old_traceback.h"

/*************Functions local to this module*************/
static void process_route(const Netlist<>& net_list, std::ifstream& fp, const char* filename, int& lineno, bool is_flat);
static void process_nodes(const Netlist<>& net_list, std::ifstream& fp, ClusterNetId inet, const char* filename, int& lineno);
static void process_nets(const Netlist<>& net_list, std::ifstream& fp, ClusterNetId inet, std::string name, std::vector<std::string> input_tokens, const char* filename, int& lineno, bool is_flat);
static void process_route(const Netlist<>& net_list,
Copy link
Contributor

Choose a reason for hiding this comment

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

The definition of some of these have doxygen comments. Can you move them to the declaration?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for breaking long lines. Can you do the same for the definition of these functions?

std::ifstream& fp,
const char* filename,
int& lineno,
bool verify_rr_switch_id,
bool is_flat);

static void process_nodes(const Netlist<>& net_list,
std::ifstream& fp,
ClusterNetId inet,
const char* filename,
bool verify_rr_switch_id,
int& lineno);

static void process_nets(const Netlist<>& net_list,
std::ifstream& fp,
ClusterNetId inet,
std::string name,
std::vector<std::string> input_tokens,
const char* filename,
int& lineno,
bool verify_rr_switch_id,
bool is_flat);

static void update_rr_switch_id(t_trace* trace, std::set<int>& seen_rr_nodes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doxygen comment


static void process_global_blocks(const Netlist<>& net_list, std::ifstream& fp, ClusterNetId inet, const char* filename, int& lineno, bool is_flat);
static void format_coordinates(int& layer_num, int& x, int& y, std::string coord, ClusterNetId net, const char* filename, const int lineno);
static void format_pin_info(std::string& pb_name, std::string& port_name, int& pb_pin_num, const std::string& input);
Expand Down Expand Up @@ -112,7 +135,7 @@ bool read_route(const char* route_file, const t_router_opts& router_opts, bool v
}

/* Read in every net */
process_route(router_net_list, fp, route_file, lineno, is_flat);
process_route(router_net_list, fp, route_file, lineno, router_opts.verify_rr_switch_id, is_flat);

fp.close();

Expand Down Expand Up @@ -145,7 +168,12 @@ bool read_route(const char* route_file, const t_router_opts& router_opts, bool v
}

///@brief Walks through every net and add the routing appropriately
static void process_route(const Netlist<>& net_list, std::ifstream& fp, const char* filename, int& lineno, bool is_flat) {
static void process_route(const Netlist<>& net_list,
std::ifstream& fp,
const char* filename,
int& lineno,
bool verify_rr_switch_id,
bool is_flat) {
std::string input;
std::vector<std::string> tokens;
while (std::getline(fp, input)) {
Expand All @@ -158,15 +186,15 @@ static void process_route(const Netlist<>& net_list, std::ifstream& fp, const ch
continue; //Skip commented lines
} else if (tokens[0] == "Net") {
ClusterNetId inet(atoi(tokens[1].c_str()));
process_nets(net_list, fp, inet, tokens[2], tokens, filename, lineno, is_flat);
process_nets(net_list, fp, inet, tokens[2], tokens, filename, lineno, verify_rr_switch_id, is_flat);
}
}

tokens.clear();
}

///@brief Check if the net is global or not, and process appropriately
static void process_nets(const Netlist<>& net_list, std::ifstream& fp, ClusterNetId inet, std::string name, std::vector<std::string> input_tokens, const char* filename, int& lineno, bool is_flat) {
static void process_nets(const Netlist<>& net_list, std::ifstream& fp, ClusterNetId inet, std::string name, std::vector<std::string> input_tokens, const char* filename, int& lineno, bool verify_rr_switch_id, bool is_flat) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is too long.

Copy link
Contributor

Choose a reason for hiding this comment

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

move the doxygen comment to declaration

Copy link
Contributor

Choose a reason for hiding this comment

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

When moving the comment, see if you can improve it. What does "process appropriately" mean?

if (input_tokens.size() > 3 && input_tokens[3] == "global"
&& input_tokens[4] == "net" && input_tokens[5] == "connecting:") {
/* Global net. Never routed. */
Expand Down Expand Up @@ -199,12 +227,12 @@ static void process_nets(const Netlist<>& net_list, std::ifstream& fp, ClusterNe
name.c_str(), size_t(inet), net_list.net_name(inet).c_str());
}

process_nodes(net_list, fp, inet, filename, lineno);
process_nodes(net_list, fp, inet, filename, verify_rr_switch_id, lineno);
}
input_tokens.clear();
}

static void process_nodes(const Netlist<>& net_list, std::ifstream& fp, ClusterNetId inet, const char* filename, int& lineno) {
static void process_nodes(const Netlist<>& net_list, std::ifstream& fp, ClusterNetId inet, const char* filename, bool verify_rr_switch_id, int& lineno) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is too long

/* Not a global net. Goes through every node and add it into trace.head*/
auto& device_ctx = g_vpr_ctx.mutable_device();
const auto& rr_graph = device_ctx.rr_graph;
Expand Down Expand Up @@ -396,12 +424,67 @@ static void process_nodes(const Netlist<>& net_list, std::ifstream& fp, ClusterN
oldpos = fp.tellg();
}

if (verify_rr_switch_id) {
VTR_ASSERT(validate_traceback(head_ptr));
} else {
std::set<int> seen_rr_nodes;
update_rr_switch_id(head_ptr, seen_rr_nodes);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like update_rr_switch_id has much of the functionality of validate_traceback inside it. I think it would be more clear to rename validate_traceback to validate_and_update_traceback and pass it a flag remap_switch_id which makes it do the remapping behaviour as it validates.

If it is safe to remap before validation (which it might not be if the traceback is invalid) you could alternatively make a more limited remapping function that you call before traceback validation.

}

/* Convert to route_tree after reading */
VTR_ASSERT(validate_traceback(head_ptr));
route_ctx.route_trees[inet] = TracebackCompat::traceback_to_route_tree(head_ptr);
free_traceback(head_ptr);
}

static void update_rr_switch_id(t_trace* trace, std::set<int>& seen_rr_nodes) {
if (!trace) {
return;
}

seen_rr_nodes.insert(trace->index);

t_trace* next = trace->next;

if (next) {
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 you can use if (next == nullptr) to return early and get rid of this if statement. Less nestedness increases readablity

if (trace->iswitch == OPEN) { //End of a branch

//Verify that the next element (branch point) has been already seen in the traceback so far
if (!seen_rr_nodes.count(next->index)) {
VPR_FATAL_ERROR(VPR_ERROR_ROUTE, "Traceback branch point %d not found", next->index);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this else statement necessary?
I think we can remove it. Then, both branches of if (trace->iswitch == OPEN) call update_rr_switch_id(next, seen_rr_nodes). So, we can move it outside the conditional statement.

//Recurse along the new branch
update_rr_switch_id(next, seen_rr_nodes);
return;
}
} else { //Midway along branch

//Check there is an edge connecting trace and next
Copy link
Contributor

Choose a reason for hiding this comment

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

To be consistent with the coding style guide, add a space after //


auto& device_ctx = g_vpr_ctx.device();
const auto& rr_graph = device_ctx.rr_graph;
Copy link
Contributor

Choose a reason for hiding this comment

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

The only usage of device_ctx is to access rr_graph.
const auto& rr_graph = g_vpr_ctx.device().rr_graph would be more concise.

bool found = false;
for (t_edge_size iedge = 0; iedge < rr_graph.num_edges(RRNodeId(trace->index)); ++iedge) {
int to_node = size_t(rr_graph.edge_sink_node(RRNodeId(trace->index), iedge));
if (to_node == next->index) {
found = true;

//Verify that the switch matches
int rr_iswitch = rr_graph.edge_switch(RRNodeId(trace->index), iedge);
trace->iswitch = rr_iswitch;
break;
}
}
if (!found) {
VPR_FATAL_ERROR(VPR_ERROR_ROUTE, "Traceback no RR edge between RR nodes %d -> %d\n", trace->index, next->index);
}
//Recurse
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 an iterative function that uses std::stack is easier to understand and eliminates the possiblity of stack overflow.
With an iterative function, you don't need to pass std::set as an argument, making the function self-contained.

update_rr_switch_id(next, seen_rr_nodes);
return;
}
}
VTR_ASSERT(!next);
}

/**
* @brief This function goes through all the blocks in a global net and verify
* it with the clustered netlist and the placement
Expand Down
11 changes: 9 additions & 2 deletions vpr/src/base/read_route.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,12 @@
#include "netlist.h"
#include "vpr_types.h"

bool read_route(const char* route_file, const t_router_opts& RouterOpts, bool verify_file_digests, bool is_flat);
void print_route(const Netlist<>& net_list, const char* placement_file, const char* route_file, bool is_flat);
bool read_route(const char* route_file,
const t_router_opts& RouterOpts,
bool verify_file_digests,
bool is_flat);

void print_route(const Netlist<>& net_list,
const char* placement_file,
const char* route_file,
bool is_flat);
5 changes: 4 additions & 1 deletion vpr/src/base/vpr_api.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1152,7 +1152,10 @@ RouteStatus vpr_load_routing(t_vpr_setup& vpr_setup,
auto& filename_opts = vpr_setup.FileNameOpts;

//Load the routing from a file
bool is_legal = read_route(filename_opts.RouteFile.c_str(), vpr_setup.RouterOpts, filename_opts.verify_file_digests, is_flat);
bool is_legal = read_route(filename_opts.RouteFile.c_str(),
vpr_setup.RouterOpts,
filename_opts.verify_file_digests,
is_flat);
const Netlist<>& router_net_list = is_flat ? (const Netlist<>&)g_vpr_ctx.atom().netlist() : (const Netlist<>&)g_vpr_ctx.clustering().clb_nlist;
if (vpr_setup.Timing.timing_analysis_enabled) {
//Update timing info
Expand Down
2 changes: 2 additions & 0 deletions vpr/src/base/vpr_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -1343,6 +1343,8 @@ struct t_router_opts {

bool with_timing_analysis;

bool verify_rr_switch_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

doxygen comment

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 you could name this better. Isn't this controlling whether you verify the switch id in route files? Hence a better name would be --verify_route_file_switch_id (and you'd change this in the command line, documentation and code).


// Options related to rr_node reordering, for testing and possible cache optimization
e_rr_node_reorder_algorithm reorder_rr_graph_nodes_algorithm = DONT_REORDER;
int reorder_rr_graph_nodes_threshold = 0;
Expand Down