Add a new API set_node_ptc_num() to RRGraphBuilder #1865

m-hariszafar · 2021-10-01T06:35:57Z

Description

This PR focuses on updating routing resource graph builder functions, where we use the refactored data structure RRGraphBuilder to shadow the discrete data structure rr_graph_storage.
This PR aims to fully deprecate the direct use of the legacy API set_ptc_num() from the rr_node data structure.

After this PR, the set_ptc_num API is fully deprecated and the set_node_ptc_num from the refactored data structure RRGraphBuilder is the only way to use it.

Checklist:

Removed the legacy API set_ptc_num() from rr_node.cpp and rr_node.h
Added new APIs set_node_ptc_num to data structures RRGraphBuilder, whose comments are Doxygen compatible
Replaced all the use of set_ptc_num() in builder functions by set_node_ptc_num()

Related Issue

This pull request is a follow-up PR on the routing resource graph refactoring effort #1805

Motivation and Context

This PR is the continuation of the refactoring effort with a focus on shadowing the rr_graph_storage APIs in the RRGraphBuilder data structure.
This PR reworks the set_node_ptc_num() API among the other APIs in #1847

How Has This Been Tested?

All vtr basic and strong tests are passing.

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

m-hariszafar · 2021-10-05T05:48:30Z

QoR tests are completed and a quick summary is given below. QoR comparison files are also attached.

titan_quick_qor test:
flow run time +2.05% on average.
critical path delay +0.26% on average.
Peak memory usage -1.8% on average

vtr_reg_qor_chain_depop test:
Flow run time -0.31% on average.
critical path delay -0.14% on average.
Peak memory usage -3.34% on average

Here are the attached files
vtr_reg_qor_chain_depop.xlsx
titan_quick_qor.xlsx

m-hariszafar · 2021-10-05T05:50:29Z

@tangxifan

tangxifan · 2021-10-07T00:31:28Z

vpr/src/device/rr_graph_builder.h

@@ -65,6 +65,13 @@ class RRGraphBuilder {
        node_storage_.set_node_coordinates(id, x1, y1, x2, y2);
    }

+    /** @brief Set the node_ptc_num; The ptc (pin, track, or class) number is an integer
+     *  that allows you to differentiate between wires, pins or sources/sinks with overlapping x,y coordinates or extent.
+     *  This is useful for drawing rr-graphs nicely.*/


The set_node_ptc_num is a tricky API because the set_ptc_num carries different meanings for different type of nodes.
You can see that there are other APIs which try to clarify this, for example,

set_node_track_num() is for CHANX and CHANY nodes

set_node_pin_num() is for OPIN and IPIN nodes

set_node_class_num() is for SOURCE and SINK nodes

Therefore, I am thinking about if we should

refactor all the APIs above in one pull request

evaluate if we should deprecate the API set_node_ptc_num() and use other APIs instead. If we cannot, I am o.k. to keep the API.

@vaughnbetz Please share your thoughts.

@hariszafar-lm I have talked to @vaughnbetz today. He agrees on the roadmap here. Please give a try. Also remember to fix the merging conflicts.

@tangxifan Sure, I'll start it from today. thanks

vaughnbetz

I think we should replace some (maybe all) set_node_ptc_num calls with calls to the more specific set_node_xx_num methods.

vaughnbetz · 2021-10-07T18:57:26Z

vpr/src/route/clock_connection_builders.cpp

@@ -106,7 +106,7 @@ RRNodeId RoutingToClockConnection::create_virtual_clock_network_sink_node(int x,
    }
    int ptc = max_ptc + 1;

-    rr_graph.set_node_ptc_num(node_index, ptc);
+    rr_graph_builder.set_node_ptc_num(node_index, ptc);


This node is a SINK, so we should be able to use the set_node_class_num() api here now. Best to avoid the set_node_ptc_num () (which is more vague) as much as possible.

vaughnbetz · 2021-10-07T18:58:07Z

vpr/src/route/rr_graph.cpp

@@ -1433,7 +1433,7 @@ static void build_rr_sinks_sources(RRGraphBuilder& rr_graph_builder,
        float R = 0.;
        float C = 0.;
        L_rr_node.set_node_rc_index(inode, find_create_rr_rc_data(R, C));
-        L_rr_node.set_node_ptc_num(inode, iclass);
+        rr_graph_builder.set_node_ptc_num(inode, iclass);


Use set_node_class_num instead (this is for SINKs and SOURCEs).

vaughnbetz · 2021-10-07T18:59:52Z

vpr/src/route/rr_graph.cpp

@@ -1671,7 +1671,7 @@ static void build_rr_chan(RRGraphBuilder& rr_graph_builder,
        float C = length * seg_details[track].Cmetal();
        L_rr_node.set_node_rc_index(node, find_create_rr_rc_data(R, C));

-        L_rr_node.set_node_ptc_num(node, track);
+        rr_graph_builder.set_node_ptc_num(node, track);


use set_node_track_num() instead.

Hello @tangxifan and @vaughnbetz,
I replaced this one with set_node_track_num() and it is firing an exception of Attempted to set RR node 'track_num' for non-CHANX/CHANY type '(null)'. Do we have an alternative or we should keep with the previous one set_node_ptc_num()?
As it is passing with set_node_ptc_num().

It is failing for all basic to strong tests.
test-runs

@hariszafar-lm This is because the set_node_type() is called after this API. If you move the LINE 1675 before this line. it should work.

vaughnbetz · 2021-10-07T19:00:07Z

vpr/src/route/rr_graph.cpp

@@ -1481,7 +1481,7 @@ static void build_rr_sinks_sources(RRGraphBuilder& rr_graph_builder,
                            float R = 0.;
                            float C = 0.;
                            L_rr_node.set_node_rc_index(inode, find_create_rr_rc_data(R, C));
-                            L_rr_node.set_node_ptc_num(inode, ipin);
+                            rr_graph_builder.set_node_ptc_num(inode, ipin);


Use set_node_pin_num() instead

vaughnbetz · 2021-10-07T19:01:41Z

vpr/src/route/rr_graph_uxsdcxx_serializer.h

@@ -604,7 +604,7 @@ class RrGraphSerializer final : public uxsd::RrGraphBase<RrGraphContextTypes> {
        RRNodeId node_id = node.id();

        rr_graph_builder_->set_node_coordinates(node_id, xlow, ylow, xhigh, yhigh);
-        node.set_ptc_num(ptc);
+        rr_graph_builder_->set_node_ptc_num(node_id, ptc);


This is the only call that really needs to set a ptc_num, no matter what type of node it is. If the node type is already set, we could replace this with an if statement that checks the type and calls the right set_node_xx_num() call, and then delete the set_node_ptc_num() method.

If that isn't easily doable, we can leave this call and the method.

@tangxifan what do you suggest on this? Either we should keep it to the set_node_ptc_num() method or else?

Yes. We should keep the set_node_ptc_num() API here

Okay, thanks for quick response

tangxifan · 2021-10-11T19:40:34Z

vpr/src/route/rr_graph.cpp

@@ -1671,7 +1671,7 @@ static void build_rr_chan(RRGraphBuilder& rr_graph_builder,
        float C = length * seg_details[track].Cmetal();
        L_rr_node.set_node_rc_index(node, find_create_rr_rc_data(R, C));

-        L_rr_node.set_node_ptc_num(node, track);
+        rr_graph_builder.set_node_ptc_num(node, track);


@hariszafar-lm This is because the set_node_type() is called after this API. If you move the LINE 1675 before this line. it should work.

tangxifan · 2021-10-11T19:41:15Z

@hariszafar-lm Also, you need to resolve the merging conflicts

tangxifan · 2021-10-11T20:31:02Z

Link to #1868 So that we keep tracking.

tangxifan · 2021-12-17T22:13:20Z

Close ad PR #1872 merged

Muhammad Haris Zafar added 5 commits September 30, 2021 15:53

Adding api set_node_ptc_num to rr_graph_builder.h

8e9df79

Replacing api set_node_ptc_num/set_ptc_num from related .cpp/.h files

d43fd06

Removing old api set_ptc_num from rr_node.cpp/.h

3184a02

Fixing typo

7db718a

Adding comments about api set_node_ptc_num

411c891

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Oct 1, 2021

m-hariszafar marked this pull request as ready for review October 5, 2021 05:48

tangxifan reviewed Oct 7, 2021

View reviewed changes

vaughnbetz requested changes Oct 7, 2021

View reviewed changes

tangxifan requested changes Oct 11, 2021

View reviewed changes

m-hariszafar mentioned this pull request Oct 13, 2021

Add a new API set_node_x_num() to RRGraphBuilder #1872

Merged

11 tasks

tangxifan mentioned this pull request Oct 14, 2021

Dashboard for RRGraph API refactoring status #1868

Open

44 tasks

tangxifan closed this Dec 17, 2021

tangxifan deleted the api_set_node_ptc_num branch December 17, 2021 22:13

Add a new API set_node_ptc_num() to RRGraphBuilder #1865

Add a new API set_node_ptc_num() to RRGraphBuilder #1865

Uh oh!

Conversation

m-hariszafar commented Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-hariszafar commented Oct 5, 2021

Uh oh!

m-hariszafar commented Oct 5, 2021

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!

m-hariszafar Oct 11, 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!

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 Oct 11, 2021

Uh oh!

tangxifan commented Oct 11, 2021

Uh oh!

tangxifan commented Dec 17, 2021

Uh oh!

Uh oh!

m-hariszafar commented Oct 1, 2021 •

edited

Loading

m-hariszafar Oct 11, 2021 •

edited

Loading