Implementation Methods for RRGraphView API

[ethanroj23]

Overview

In my implementation of API function calls for the RRGraphView class, I have come up with several methods that could be used. Before adding several hundreds of function calls, I figured it would be good to have some input. The three methods are included below. I am currently leaning towards method 2 or 3. Soon after Xifan's PR (#1693) is merged, I will make a PR which will add these calls to RRGraphView::node_type(RRNodeId node).

@vaughnbetz, @hzeller, @tangxifan, @nelsobe, which of these methods do you think is best? Or is there another method that is better?

API Implementation Variations (check_route.cpp starting at line 200)

1. Single Line Replacement

static void check_source(int inode, ClusterNetId net_id) {
    auto& device_ctx = g_vpr_ctx.device();
    auto& cluster_ctx = g_vpr_ctx.clustering();
    auto& place_ctx = g_vpr_ctx.placement();

-   t_rr_type rr_type = device_ctx.rr_nodes[inode].type();
+   t_rr_type rr_type = device_ctx.rr_graph.node_type(RRNodeId(inode));
    if (rr_type != SOURCE) {
        VPR_FATAL_ERROR(VPR_ERROR_ROUTE,
                        "in check_source: net %d begins with a node of type %d.\n", size_t(net_id), rr_type);
    }

2. Addition of const auto& rr_graph

static void check_source(int inode, ClusterNetId net_id) {
    auto& device_ctx = g_vpr_ctx.device();
+   const auto& rr_graph = device_ctx.rr_graph;
    auto& cluster_ctx = g_vpr_ctx.clustering();
    auto& place_ctx = g_vpr_ctx.placement();

-   t_rr_type rr_type = device_ctx.rr_nodes[inode].type();
+   t_rr_type rr_type = rr_graph.node_type(RRNodeId(inode));
    if (rr_type != SOURCE) {
        VPR_FATAL_ERROR(VPR_ERROR_ROUTE,
                        "in check_source: net %d begins with a node of type %d.\n", size_t(net_id), rr_type);
    }

3. Addition of const auto& rr_graph and an additional variable for the RRNodeId

static void check_source(int inode, ClusterNetId net_id) {
    auto& device_ctx = g_vpr_ctx.device();
+   const auto& rr_graph = device_ctx.rr_graph;
    auto& cluster_ctx = g_vpr_ctx.clustering();
    auto& place_ctx = g_vpr_ctx.placement();

+   RRNodeId inode_i = RRNodeId(inode);

-   t_rr_type rr_type = device_ctx.rr_nodes[inode].type();
+   t_rr_type rr_type = rr_graph.node_type(inode_i);
    if (rr_type != SOURCE) {
        VPR_FATAL_ERROR(VPR_ERROR_ROUTE,
                        "in check_source: net %d begins with a node of type %d.\n", size_t(net_id), rr_type);
    }

[vaughnbetz]

I'd go for 1 or 2 (no strong preference between them on my part). I don't like #3 as the cast to RRNodeId is pretty mechanical and adding a temporary for it just creates additional names to keep track of.

In some cases you may be able to pass in an RRNodeId (change the type of the input parameter) to avoid the conversion to RRNodeId. If that can be done without undue effort it is better, as it expands the type-checking back to the caller, where it is more useful.

[tangxifan]

I agree with @vaughnbetz that 1 and 2 are better than 3.
Between 1 and 2, I prefer 2 because it is easier for future refactoring (in my opinion).
2 allows us to easily add RRGraphView as an input argument of the function when we plan to get rid of global variable usage in these functions.

[nelsobe]

I agree with both of the above in terms of #1 or #2 being preferable to #3. I would vote for #2 given a choice but it is not super strong.

[vaughnbetz]

I say we go for #2 then to make future refactoring easier, and since you find it more readable Ethan.

[ethanroj23]

We have decided on method #2 for now.