-
Notifications
You must be signed in to change notification settings - Fork 414
Add a new API set_node_rc_index() to RRGraphBuilder #1887
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
Add a new API set_node_rc_index() to RRGraphBuilder #1887
Conversation
QoR tests are completed and a quick summary is given below. QoR comparison files are also attached. titan_quick_qor test: vtr_reg_qor_chain_depop test: Here are the attached files |
After fixing the spreadsheet manually (removing the newly added columns and rerunning the compare script) titan_quick_qor test: vtr_reg_qor_chain_depop test: Here are the attached files |
vpr/src/device/rr_graph_builder.h
Outdated
@@ -102,6 +102,11 @@ class RRGraphBuilder { | |||
node_storage_.set_node_direction(id, new_direction); | |||
} | |||
|
|||
/** @brief Set the rc_index of routing resource node. */ | |||
inline void set_node_rc_index(RRNodeId id, short new_rc_index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hariszafar-lm I had a discussion with @vaughnbetz . We believe it is better to use a strong id here, e.g., NodeRcIndex, rather than an integer. Can you look into the implementation here:
struct rr_indexed_data_id_tag; |
And then create a strong id for the RC index? For internal data of RRGraphBuilder, you can still use integer.
But for the API, we force the use of strong ids.
Let me know if this makes sense or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tangxifan, could you review it now. I had created and implemented strong id for it in this commit.
vpr/src/route/rr_graph.cpp
Outdated
@@ -1437,7 +1437,7 @@ static void build_rr_sinks_sources(RRGraphBuilder& rr_graph_builder, | |||
rr_graph_builder.set_node_coordinates(inode, i, j, i + type->width - 1, j + type->height - 1); | |||
float R = 0.; | |||
float C = 0.; | |||
L_rr_node.set_node_rc_index(inode, find_create_rr_rc_data(R, C)); | |||
rr_graph_builder.set_node_rc_index(inode, find_create_rr_rc_data(R, C)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hariszafar-lm If you see any functions in rr_graph.cpp and rr_graph2.cpp which no longer use the old data structure L_rr_node
, please take a note. We can create a separated pull request where we rework the input argument list by using RRGraphBuilder
and RRGraphView
. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, I'll create a separate PR for this.
@@ -114,7 +114,7 @@ RRNodeId RoutingToClockConnection::create_virtual_clock_network_sink_node(int x, | |||
|
|||
float R = 0.; | |||
float C = 0.; | |||
rr_graph.set_node_rc_index(node_index, find_create_rr_rc_data(R, C)); | |||
rr_graph_builder.set_node_rc_index(node_index, NodeRCIndex(find_create_rr_rc_data(R, C))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hariszafar-lm Can you quickly check if we can change the data type that function find_create_rr_rc_data()
returns? We can avoid these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that before but it gives a lot of errors related to conversions and ambiguating new declaration of NodeRCIndex find_create_rr_rc_data(float, float)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. Can you share the output of grep -r "find_create_rr_rc_data" .
? So that I know how many times it is called in all the source files. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the output on the master branch. It has been calling 11 times.
vpr/src/route/clock_connection_builders.cpp: rr_graph.set_node_rc_index(node_index, find_create_rr_rc_data(R, C));
vpr/src/route/clock_network_builders.cpp: node.set_rc_index(find_create_rr_rc_data(
vpr/src/route/clock_network_builders.cpp: node.set_rc_index(find_create_rr_rc_data(
vpr/src/route/rr_graph.cpp: L_rr_node.set_node_rc_index(inode, find_create_rr_rc_data(R, C));
vpr/src/route/rr_graph.cpp: L_rr_node.set_node_rc_index(inode, find_create_rr_rc_data(R, C));
vpr/src/route/rr_graph.cpp: L_rr_node.set_node_rc_index(node, find_create_rr_rc_data(R, C));
vpr/src/route/rr_graph_timing_params.cpp: mutable_device_ctx.rr_nodes[inode].set_rc_index(find_create_rr_rc_data(rr_graph.node_R(RRNodeId(inode)), rr_node_C[inode]));
vpr/src/route/rr_graph_uxsdcxx_serializer.h: node.set_rc_index(find_create_rr_rc_data(R, C));
vpr/src/route/rr_graph_uxsdcxx_serializer.h: node.set_rc_index(find_create_rr_rc_data(0, 0));
vpr/src/route/rr_rc_data.cpp:short find_create_rr_rc_data(const float R, const float C) {
vpr/src/route/rr_rc_data.h:short find_create_rr_rc_data(const float R, const float C);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hariszafar-lm This is not big. If you want to avoid big changes in this PR. We can do it in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll look at this in some other PR.
@hariszafar-lm Please resolve the conflicts. |
@vaughnbetz @hariszafar-lm The doc build failure may be due to a dependency issue: #1898 Since this PR does not change documentation settings, I think we should merge. I will be on the doc issue if we see anything coming from this PR. |
Description
This PR focuses on updating routing resource graph builder functions, where we use the refactored data structure
RRGraphBuilder
to shadow the discrete data structurerr_graph_storage
.This PR aims to fully refactored/deprecate the direct use of the legacy API
set_node_rc_index()
from therr_node
data structure.After this PR, the
set_rc_index()
API is fully deprecated, and theset_node_rc_index()
from the refactored data structureRRGraphBuilder
is the only way to use it.Checklist:
set_rc_index()
fromrr_node.cpp
andrr_node.h
set_node_rc_index
to data structuresRRGraphBuilder
, whose comments are Doxygen compatibleset_rc_index()
with respectiveset_node_rc_index
in builder functionsRelated Issue
This pull request is a follow-up PR on the routing resource graph refactoring effort #1805, #1868
Motivation and Context
This PR is a continuation of the refactoring effort with a focus on shadowing the
rr_graph_storage
APIs in theRRGraphBuilder
data structure.This PR refactored the
set_node_rc_index()
API among the other APIs in #1847, #1868How Has This Been Tested?
Types of changes
Checklist: