RR Edge ID Verification #3164

amin1377 · 2025-06-24T22:41:26Z

Description

In a .route file, each edge between two nodes includes a switch ID, which is used to retrieve the electrical characteristics of that edge. When reading the .route file, this switch ID is compared against the corresponding switch ID from the RR Graph for consistency. If they do not match, an error is raised.

This consistency check is generally helpful. However, when analyzing multiple timing corners, each corner may have a different RR Graph with varying timing information (graph topologies are the same). As a result, the number and ordering of switch types may differ between corners. To allow reuse of the same .route file for fair comparisons across different timing corners, this strict verification can become a limitation.

To address this issue, a new CLI parameter verify_rr_switch_id has been introduced. By default, it is set to on, enforcing the consistency check. When set to off, the switch ID from the .route file is ignored, and the switch ID from the RR Graph is used instead.

soheilshahrouz

Thanks @amin1377.

I left a few comment for improving the readability a bit.

soheilshahrouz · 2025-06-25T15:33:53Z

vpr/src/base/read_options.cpp

@@ -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")
+        .help(
+            "Verify that the switch IDs in the routing file are consistent with those in the RR Graph."


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

document this new option in the rst file

soheilshahrouz · 2025-06-25T15:36:43Z

vpr/src/base/read_options.cpp

@@ -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")


If this is a routing option, add it to route_grp

soheilshahrouz · 2025-06-25T15:38:03Z

vpr/src/base/read_options.h

@@ -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;


I think this option is only applicable to the routing stage and its variable should move there

soheilshahrouz · 2025-06-25T15:40:07Z

vpr/src/base/read_route.cpp

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


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

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

soheilshahrouz · 2025-06-25T15:41:27Z

vpr/src/base/read_route.cpp

+                         bool verify_rr_switch_id,
+                         bool is_flat);
+
+static void update_rr_switch_id(t_trace* trace, std::set<int>& seen_rr_nodes);


Doxygen comment

soheilshahrouz · 2025-06-25T16:27:58Z

vpr/src/base/read_route.cpp

+            //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 {


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.

soheilshahrouz · 2025-06-25T16:30:24Z

vpr/src/base/read_route.cpp

+            //Check there is an edge connecting trace and next
+
+            auto& device_ctx = g_vpr_ctx.device();
+            const auto& rr_graph = device_ctx.rr_graph;


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.

soheilshahrouz · 2025-06-25T16:32:24Z

vpr/src/base/read_route.cpp

+            }
+        } else { //Midway along branch
+
+            //Check there is an edge connecting trace and next


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

soheilshahrouz · 2025-06-25T16:39:07Z

vpr/src/base/vpr_types.h

@@ -1343,6 +1343,8 @@ struct t_router_opts {

    bool with_timing_analysis;

+    bool verify_rr_switch_id;


doxygen comment

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

soheilshahrouz · 2025-06-25T16:45:35Z

vpr/src/base/read_route.cpp

+            if (!found) {
+                VPR_FATAL_ERROR(VPR_ERROR_ROUTE, "Traceback no RR edge between RR nodes %d -> %d\n", trace->index, next->index);
+            }
+            //Recurse


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.

vaughnbetz

Seems like there may be some code duplication ... I suggest some changes.

vaughnbetz · 2025-06-27T23:19:00Z

vpr/src/base/read_route.cpp

        }
    }

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


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

vaughnbetz · 2025-06-27T23:23:43Z

vpr/src/base/read_route.cpp

+        VTR_ASSERT(validate_traceback(head_ptr));
+    } else {
+        std::set<int> seen_rr_nodes;
+        update_rr_switch_id(head_ptr, seen_rr_nodes);


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.

vaughnbetz · 2025-06-27T23:27:02Z

vpr/src/base/vpr_types.h

@@ -1343,6 +1343,8 @@ struct t_router_opts {

    bool with_timing_analysis;

+    bool verify_rr_switch_id;


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

amin1377 added 3 commits June 24, 2025 17:18

[vpr][CLI] add verify_rr_switch_id option

44b1948

[vpr][route] pass verify_rr_switch_id to relevant functions

c1f697e

[vpr][base] add update_rr_switch_id

a5899e3

amin1377 requested review from vaughnbetz and soheilshahrouz June 24, 2025 22:41

github-actions bot added VPR VPR FPGA Placement & Routing Tool lang-cpp C/C++ code labels Jun 24, 2025

soheilshahrouz requested changes Jun 25, 2025

View reviewed changes

vaughnbetz requested changes Jun 27, 2025

View reviewed changes

		@@ -1343,6 +1343,8 @@ struct t_router_opts {

		bool with_timing_analysis;

		bool verify_rr_switch_id;

RR Edge ID Verification #3164

Are you sure you want to change the base?

RR Edge ID Verification #3164

Uh oh!

Conversation

amin1377 commented Jun 24, 2025

Description

Uh oh!

soheilshahrouz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaughnbetz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!