RRGraphView edge_sink_node()/edge_switch() Implementation #1930

umariqbal-rs · 2021-12-03T06:28:18Z

Description

In this PR, I am implementing RRGraphView::edge_switch(const RREdgeId& edge) and RRGraphView::edge_switch(RRNodeId id, t_edge_size iedge) throughout VTR. Every time device_ctx.rr_nodes[from_node].edge_switch(iedge); was called has been replaced with rr_graph.edge_switch(EdgeId). In order to do this, I followed a pattern similar to that in the last PR.

Related Issue

Motivation and Context

These changes are a continuation of the RRGraphView API implementation effort

How Has This Been Tested?

Types of changes

Bug fix (change which fixes an issue)
New feature (change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation
I have updated the documentation accordingly
I have added tests to cover my changes
All new and existing tests passed

umariqbal-rs · 2021-12-03T06:31:12Z

@tangxifan
I have created edge_switch() api in rr_grap_view.h and removed it from rr_node_impl.h.

vtr-verilog-to-routing/vpr/src/device/rr_graph_view.h

Line 252 in 975e37d

inline short edge_switch(RRNodeId id, t_edge_size iedge) const {

Can Please review it and suggest me how we will move forward?

tangxifan · 2021-12-03T07:08:48Z

vpr/src/draw/draw.cpp

@@ -1860,7 +1860,8 @@ static void draw_rr_edges(int inode, ezgl::renderer* g) {
                        } else {
                            g->set_color(blk_DARKGREEN);
                        }
-                        switch_type = device_ctx.rr_nodes[inode].edge_switch(iedge);
+                        switch_type = rr_graph.edge_switch(RRNodeId(inode), iedge);
+                        ;


Redundant ;

vpr/src/route/rr_graph_timing_params.cpp

vpr/src/route/rr_node_impl.h

vpr/src/util/vpr_utils.cpp

tangxifan · 2021-12-03T07:19:28Z

vpr/src/route/rr_node.cpp

    auto& device_ctx = g_vpr_ctx.device();
-
+    const auto& rr_graph = device_ctx.rr_graph;


Can you double check the dependencies among rr_node.cpp and rr_graph_storage.cpp and rr_node_impl.h?>
My understanding, edge_switch() is a method of t_rr_node. We do not need to use the RRGraphView API here.

edge_switch() is a method of t_rr_node which is being overridden in rr_node_impl.h. We are introducing the APIs in rr_graph_view.h to delete the functions of rr_node_impl.h. Once #1917 is merged, we will move edge_is_configurable() and validate() from rr_node.cpp to rr_graph_view.h as we are trying to shadow the rr_node.cpp by introducing APIs to rr_graph_view.h

Make sense. I am fine with this plan.

Please leave a comment

/* TODO: This API should be reworked once rr_switch APIs are in RRGraphView. */

vpr/src/route/router_lookahead_map_utils.cpp

vpr/src/route/router_lookahead_extended_map.cpp

vpr/src/device/rr_graph_view.h

tangxifan · 2021-12-06T06:50:25Z

@umariqbal-rs Please check failures in CI tests

…node_impl.h.

umariqbal-rs · 2021-12-06T12:52:02Z

@tangxifan
I have created edge_sink_node api in rr_graph_view.h

vtr-verilog-to-routing/vpr/src/device/rr_graph_view.h

Line 261 in 8a3abba

inline RRNodeId edge_sink_node(RRNodeId id, t_edge_size iedge) const {

We need to type cast and we can't type cast int with RRNodeId so I typed casted it like this

vtr-verilog-to-routing/vpr/src/route/route_tree_timing.cpp

Line 424 in 8a3abba

int to_rr_node = size_t(rr_graph.edge_sink_node(RRNodeId(rr_node), iedge));

Can you Please review it, Is that ok? So we can move forward.

tangxifan · 2021-12-06T18:42:11Z

@umariqbal-rs The type cast is correct. No worries.

umariqbal-rs · 2021-12-06T18:47:06Z

That is great. I will complete tomorrow this API.

umariqbal-rs · 2021-12-08T13:04:45Z

@tangxifan
Can you Please review it?

tangxifan

@umariqbal-rs Just did a deep dive here. See a few places where we can streamline our codes.

vpr/src/device/rr_graph_view.h

tangxifan · 2021-12-08T17:43:43Z

vpr/src/draw/draw.cpp

@@ -1689,7 +1689,7 @@ static void draw_rr_edges(int inode, ezgl::renderer* g) {
    from_ptc_num = rr_graph.node_ptc_num(RRNodeId(inode));

    for (t_edge_size iedge = 0, l = device_ctx.rr_nodes[inode].num_edges(); iedge < l; iedge++) {
-        to_node = device_ctx.rr_nodes[inode].edge_sink_node(iedge);
+        to_node = size_t(rr_graph.edge_sink_node(RRNodeId(inode), iedge));


I just read the codes of this function. It seems that we can eliminate the use of type casting RRNodeId(inode) and RRNodeId(to_node).
In LINE 1675, you can see the rr_node variable is already defined.
In LINE 1678, you can change the data type of to_node from int to RRNodeId

Then in the rest of this function, you just replace

RRNodeId(inode) -> rr_node

RRNodeId(to_node) -> to_node.

We can do it in a follow-up PR.

Let me know what you think.

But if we see at

vtr-verilog-to-routing/vpr/src/draw/draw.cpp

Line 1725 in 026c13c

draw_pin_to_pin(inode, to_node, g);

here in this function argument to_node is as int type. Still RRNodeId(to_node) -> to_node Can be done ?
We can do this by defining a new variable like auto to_node_temp=RRNodeId(inode) So we should by this way ?

@umariqbal-rs You can try draw_pin_to_pin(inode, size_t(to_node), g);

@umariqbal-rs Can you let me know how many times we have to do the type casting? If it is too much, we can stay with what we have.

@tangxifan
Type casting with size_t(to_node) is more than the RRNodeId(to_node) so We should stay with the what we have.
Or We should do ?

We can stay with what we have now.

So should I go on running QoR tests?

tangxifan · 2021-12-08T17:48:35Z

vpr/src/place/timing_place_lookup.cpp

            if (rr_graph.node_type(RRNodeId(ipin_rr_node)) != IPIN) continue;

            for (t_edge_size i_ipin_edge = 0; i_ipin_edge < rr_nodes[ipin_rr_node].num_edges(); ++i_ipin_edge) {
-                if (sink_rr_node == rr_nodes[ipin_rr_node].edge_sink_node(i_ipin_edge)) {
+                if (size_t(sink_rr_node) == size_t(rr_graph.edge_sink_node(RRNodeId(ipin_rr_node), i_ipin_edge))) {


In the for loop (starting from LINE 1169), we can change the data type of

opin_rr_node from int to RRNodeId

ipin_rr_node from int to RRNodeId

I see in LINE 1174, the num_edges() remain using an old API. I believe that you have patched these line in another PR #1917
It means that we should merge #1917 before this PR.
Let me know if this is fine for you or not.

vpr/src/power/power.cpp

vpr/src/route/check_rr_graph.cpp

vpr/src/route/rr_graph_area.cpp

…-routing into HEAD updating with master .

tangxifan

@umariqbal-rs Changes look good. Just minor comments left. Can you start QoR tests?

vpr/src/draw/draw.cpp

tangxifan · 2021-12-10T17:49:58Z

vpr/src/power/power.cpp

-            if (node.edge_sink_node(edge_idx) != OPEN) {
-                if (rr_node_power[node.edge_sink_node(edge_idx)].driver_switch_type == OPEN) {
-                    rr_node_power[node.edge_sink_node(edge_idx)].driver_switch_type = node.edge_switch(edge_idx);
+            if (size_t(rr_graph.edge_sink_node(RRNodeId(rr_node_idx), edge_idx)) != size_t(OPEN)) {


Using the new API, you do not need the OPEN. This can be simplified as

if (rr_graph.edge_sink_node(RRNodeId(rr_node_idx), edge_idx)) {

Sure I will do that on Monday.

tangxifan · 2021-12-10T17:54:27Z

vpr/src/route/rr_node.cpp

    auto& device_ctx = g_vpr_ctx.device();
-
+    const auto& rr_graph = device_ctx.rr_graph;


Make sense. I am fine with this plan.

tangxifan · 2021-12-14T07:00:54Z

@umariqbal-rs Waiting for the QoR tests. Code changes look good.

tangxifan

It seems that @umariqbal-rs you reverted back to old APIs when resolving merging conflicts.

vpr/src/draw/draw.cpp

tangxifan · 2021-12-16T05:19:07Z

vpr/src/place/timing_place_lookup.cpp

@@ -1166,17 +1166,17 @@ bool directconnect_exists(int src_rr_node, int sink_rr_node) {
    VTR_ASSERT(rr_graph.node_type(RRNodeId(src_rr_node)) == SOURCE && rr_graph.node_type(RRNodeId(sink_rr_node)) == SINK);

    //TODO: This is a constant depth search, but still may be too slow
-    for (t_edge_size i_src_edge = 0; i_src_edge < rr_graph.num_edges(RRNodeId(src_rr_node)); ++i_src_edge) {
-        int opin_rr_node = rr_nodes[src_rr_node].edge_sink_node(i_src_edge);
+    for (t_edge_size i_src_edge = 0; i_src_edge < rr_nodes[src_rr_node].num_edges(); ++i_src_edge) {


vpr/src/power/power.cpp

vpr/src/route/route_common.cpp

vpr/src/route/router_lookahead_map_utils.cpp

vpr/src/route/rr_graph_timing_params.cpp

umariqbal-rs · 2021-12-17T08:40:23Z

@tangxifan
I have encountered a warning

vtr-verilog-to-routing/vpr/src/route/router_lookahead_map_utils.cpp

Line 266 in c4beebe

auto& parent_node = rr_nodes[size_t(parent)];

The parent_node is unused here.Its ok I can remove it but than

vtr-verilog-to-routing/vpr/src/route/router_lookahead_map_utils.cpp

Line 255 in c4beebe

void expand_dijkstra_neighbours(const t_rr_graph_storage& rr_nodes,

rr_nodes become unused.
If we

vtr-verilog-to-routing/vpr/src/route/router_lookahead_map.cpp

Line 649 in 6aa5209

    
           static void expand_dijkstra_neighbours(PQ_Entry parent_entry, vtr::vector<RRNodeId, float>& node_visited_costs, vtr::vector<RRNodeId, bool>& node_expanded, std::priority_queue<PQ_Entry>& pq) {

In above function is used like this

vtr-verilog-to-routing/vpr/src/route/router_lookahead_map.cpp

Line 652 in 6aa5209

auto& rr_graph = device_ctx.rr_nodes;

and also edge_sink_node and edge_switch are not using API from rr_graph_view.h

vtr-verilog-to-routing/vpr/src/route/router_lookahead_map.cpp

Line 657 in 6aa5209

RRNodeId child_node = rr_graph.edge_sink_node(edge);

Can you suggest me how to remove this warning?

tangxifan · 2021-12-17T19:36:40Z

@umariqbal-rs The warning can be avoided by deleting LINE 266. The variable parent_node is now unused in the function. It can be safely removed.

Also since this PR will finish all the edge-related APIs, it is safe to remove the workarounds at

vtr-verilog-to-routing/vpr/src/route/router_lookahead_map.cpp

Lines 651 to 652 in 6aa5209

    
           const auto& temp_rr_graph = device_ctx.rr_graph; //TODO rename to rr_graph once the rr_graph below is unneeded 
        
           auto& rr_graph = device_ctx.rr_nodes;

where you can

remove LINE 652
rename temp_rr_graph to rr_graph in LINE 651.

umariqbal-rs · 2021-12-18T15:48:55Z

@tangxifan
I have deleted parent_node

vtr-verilog-to-routing/vpr/src/route/router_lookahead_map_utils.cpp

Line 266 in c4beebe

auto& parent_node = rr_nodes[size_t(parent)];

But still we got unused variable from line 255 rr_nodes

vtr-verilog-to-routing/vpr/src/route/router_lookahead_map_utils.cpp

Line 255 in c4beebe

void expand_dijkstra_neighbours(const t_rr_graph_storage& rr_nodes,

How can I remove this variable or avoid this warning? I need your suggestion.

tangxifan · 2021-12-18T18:35:46Z

@umariqbal-rs Yes. Please remove the unused argument. It may cause large code changes. Can you do a grep on the expand_dijkstra_neighbours? See how many lines will be expected.

umariqbal-rs · 2021-12-19T00:45:39Z

@tangxifan
I have already done this and removed. But I got some unknown error like in compute function .

tangxifan · 2021-12-19T01:34:54Z

@umariqbal-rs Would you mind sharing more details, such as error messages?

umariqbal-rs · 2021-12-20T11:12:49Z

@tangxifan
We have removed warnings.
We can't remove temp_rr_graph from here

vtr-verilog-to-routing/vpr/src/route/router_lookahead_map.cpp

Line 651 in 6aa5209

    
           const auto& temp_rr_graph = device_ctx.rr_graph; //TODO rename to rr_graph once the rr_graph below is unneeded

because in
for (RREdgeId edge : rr_graph.edge_range(parent)) {
we don't have API of edge_range yet.

tangxifan

@umariqbal-rs Some final comments.

tangxifan · 2021-12-20T18:54:32Z

vpr/src/route/rr_node.cpp

    auto& device_ctx = g_vpr_ctx.device();
-
+    const auto& rr_graph = device_ctx.rr_graph;


Please leave a comment

/* TODO: This API should be reworked once rr_switch APIs are in RRGraphView. */

tangxifan · 2021-12-20T18:55:47Z

vpr/src/route/router_lookahead_map_utils.cpp

@@ -288,15 +285,13 @@ void expand_dijkstra_neighbours(const t_rr_graph_storage& rr_nodes,
    }
 }

-template void expand_dijkstra_neighbours(const t_rr_graph_storage& rr_nodes,
-                                         const PQ_Entry_Delay& parent_entry,
+template void expand_dijkstra_neighbours(const PQ_Entry_Delay& parent_entry,


We should avoid the use of global variables.
Can you just replace the const t_rr_graph_storage& rr_nodes, with const RRGraphView& rr_graph?

We have already removed rr_nodes from here. Because that was unused. And if it is used in some other files then edge_range API should be in rr_graph to remove rr_nodes from those files, as I mentioned above in comment.

Removing the rr_nodes is correct. Can you bring the RRGraphView to the input argument list of this function?

For edge_range, I will read more codes and tell you how to proceed. For this PR, it seems o.k. We will rework it in later PRs. Let me know if you have any question.

OK that is fine. Today in morning I will do that. I have already run the QoR tests should I run again after putting this argument.

Yes. We need to rerun the regression because this function is called many times in aglorithms.
Also, feel free share existing QoR test results. I can take a look.

These are results before adding RRGraphView& rr_graph in arguments.

vtr_reg_qor_chain_depop test
Flow run time -0.1% on average.
critical path delay 0% on average.
Peak memory usage 0.03% on average

titan_quick_qor test:
flow run time 3.65% on average.
critical path delay 0% on average.
Peak memory usage 0% on average

titan_quick_qor.xlsx
vtr_reg_qor_chain_depop.xlsx

The QoR result looks good. is it against current master or a specific branch test_basic_sanity?
I think the final QoR should also look fine.

Its against test_basic_sanity.

tangxifan

@umariqbal-rs Good to go from my side. Please upload the latest QoR test results. Once CI is green, I will propose to merge. Thank you for hard working.

umariqbal-rs · 2021-12-21T20:19:29Z

@tangxifan
It's my pleasure.
CI is already green .In morning I will upload new results.

tangxifan · 2021-12-21T20:20:26Z

@tangxifan It's my pleasure. CI is already green .In morning I will upload new results.

Yes. Looking forward to the new results.

umariqbal-rs · 2021-12-22T05:06:25Z

@tangxifan
vtr_reg_qor_chain_depop test:

Flow run time 1.51% on average.
critical path delay 0% on average.
Peak memory usage 1.57% on average

titan_quick_qor test:

flow run time 12.21% on average.
critical path delay 0% on average.
Peak memory usage 0.02% on average

titan_quick_qor.xlsx
vtr_reg_qor_chain_depop.xlsx

tangxifan · 2021-12-22T06:22:04Z

@umariqbal-rs Can you also run the test against current master? The 12% flow time overhead looks like a concern.

umariqbal-rs · 2021-12-22T06:26:30Z

@tangxifan
sure I am doing.

umariqbal-rs · 2021-12-23T13:05:23Z

@tangxifan
Below are the QoR Results with current master:
vtr_reg_qor_chain_depop test:
Flow run time 2.05% on average.
critical path delay 0% on average.
Peak memory usage -8.83% on average

titan_quick_qor test:

flow run time -11.59% on average.
critical path delay 0% on average.
Peak memory usage 0.02% on average
titan_quick_qor.xlsx
vtr_reg_qor_chain_depop.xlsx

tangxifan

@umariqbal-rs Thanks for the good work.
@vaughnbetz I propose to merge this PR, because

Code changes are clean (I have performed 3 rounds of code reviews)
QoR tests show no overhead

If you see any issues, please feel free to comment. We will address it in a follow PR.

edge_switch api created and removed from node_impl.h

975e37d

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Dec 3, 2021

formatting has been set

1f27d87

tangxifan requested changes Dec 3, 2021

View reviewed changes

comments and required chanings has been done

2203367

umariqbal-rs requested a review from tangxifan December 6, 2021 05:55

edge_switch() API implemented in rr_graph_view.h and deleted from rr_…

6d73d49

…node_impl.h.

behzadmehmood-rs assigned umariqbal-rs and behzadmehmood-rs Dec 6, 2021

umariqbal-rs added 2 commits December 6, 2021 15:58

formaating and warnings has been fixed

f28874f

created edge_sink_node

8a3abba

umariqbal-rs added 3 commits December 8, 2021 12:43

edge_sink_node api created and changes in respective functions

e76f49c

warnings resolved

ddaf9b1

removed from rr_node_impl.h

026c13c

tangxifan requested changes Dec 8, 2021

View reviewed changes

required changes done

272d531

behzadmehmood-rs reviewed Dec 9, 2021

View reviewed changes

vpr/src/route/rr_graph_area.cpp Outdated Show resolved Hide resolved

umariqbal-rs added 2 commits December 9, 2021 15:16

Merge branch 'master' of github.com:verilog-to-routing/vtr-verilog-to…

5fda3ee

…-routing into HEAD updating with master .

upstream

3d8c9e9

tangxifan mentioned this pull request Dec 9, 2021

Dashboard for RRGraph API refactoring status #1868

Open

44 tasks

required changes has been done

e3c30e0

umariqbal-rs requested a review from tangxifan December 10, 2021 15:58

tangxifan reviewed Dec 10, 2021

View reviewed changes

symbiflow-robot removed the kokoro:force-run label Dec 16, 2021

tangxifan requested changes Dec 16, 2021

View reviewed changes

resolved num_edges

d2de2b0

umariqbal-rs requested a review from tangxifan December 17, 2021 06:11

with a warning

c4beebe

tangxifan mentioned this pull request Dec 17, 2021

RRGraphView nodes() Implementation #1932

Merged

7 tasks

removed unsused argument

bccca16

tangxifan requested changes Dec 20, 2021

View reviewed changes

final comments and rr_graph argument has been added

52a60bf

umariqbal-rs added the kokoro:force-run label Dec 21, 2021

symbiflow-robot removed the kokoro:force-run label Dec 21, 2021

umariqbal-rs requested a review from tangxifan December 21, 2021 20:13

tangxifan reviewed Dec 21, 2021

View reviewed changes

umariqbal-rs requested a review from tangxifan December 23, 2021 17:45

tangxifan approved these changes Dec 23, 2021

View reviewed changes

tangxifan merged commit 1333ba7 into verilog-to-routing:master Dec 23, 2021

tangxifan deleted the edge_switch/edge_sink_node branch December 23, 2021 18:44

		auto& device_ctx = g_vpr_ctx.device();

		const auto& rr_graph = device_ctx.rr_graph;

RRGraphView edge_sink_node()/edge_switch() Implementation #1930

RRGraphView edge_sink_node()/edge_switch() Implementation #1930

Uh oh!

Conversation

umariqbal-rs commented Dec 3, 2021

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

umariqbal-rs commented Dec 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tangxifan commented Dec 6, 2021

Uh oh!

umariqbal-rs commented Dec 6, 2021

Uh oh!

tangxifan commented Dec 6, 2021

Uh oh!

umariqbal-rs commented Dec 6, 2021

Uh oh!

umariqbal-rs commented Dec 8, 2021

Uh oh!

tangxifan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

umariqbal-rs Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tangxifan left a comment

Choose a reason for hiding this comment

Uh oh!

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!

tangxifan commented Dec 14, 2021

Uh oh!

umariqbal-rs Dec 9, 2021 •

edited

Loading

umariqbal-rs Dec 20, 2021 •

edited

Loading