Open tangxifan opened 3 years ago
@ethanroj23 @hariszafar-lm @hamzakhan-rs I summarized our ongoing refactoring effort here and created a checklist. The idea is to keep a memo for all of us on the progress bar, so that we all know where we are and how far we are to reach the destination.
@tangxifan There are some new APIs which do not exist in rr_node.h or anywhere you mentioned above and we will implement them in RRGraphView or RRGraphBuilder, right? @ethanroj23 Can you please tell me which APIs you are currently working on? So that we can work on others.
@baberali-rs I am currently working on node_type()
, node_type_string()
, and node_side_string()
in RRGraphView.
@ethanroj23 Thanks for the update. While are you working these APIs, would you mind the Rapidsilicon team working on merging the rr_segment
into RRGraphView? We will be active in resolving any merging conflicts.
@tangxifan I would not mind. That would be very helpful, thanks!
@baberali-rs Yes. I have discussed with @vaughnbetz and @kmurray. We agree to
rr_index_data
, rr_switch
and rr_segment
. For any node/edge-level data query API in RRGraphBuilder
and RRGraphView
, it should return the strong id rather than an int
as the unique index. rr_index_data
, rr_switch
and rr_segment
into RRGraphBuilder
and RRGraphView
as an internal storage. It will be part of the internal data at
https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/7ec9f743bce637264f7cba4e13f3811df2692b5c/vpr/src/device/rr_graph_builder.h#L105-L119rr_index_data
, rr_switch
and rr_segment
, e.g., seg_type(RRNodeId)
with a given node id, rather than a two-step process (get RRSegmentId
first and then get the data from rr_segments). @vaughnbetz mentioned it may cause some CPU penalties. But we do not know if it is going to be trivial or huge. If trivial, I believe it is worthy.See details in https://github.com/verilog-to-routing/vtr-verilog-to-routing/pull/1843
I believe you and your team can start developing the step 1 & 2 in the checklist and create pull requests.
Let me know if you have any questions.
As discussed with @vaughnbetz, once the API refactoring is done. We should do the follow-up refactoring
librrgraph
. Then VPR will use linker to access the data structures
RRGraphBuilder
, RRGraphView
and RRGraphReader
and RRGraphWriter
@vaughnbetz If I misunderstood your message, feel free to comment. I will formulate the problem.
As discussed with @ethanroj23 , some of the remaining tasks on the APIs will be assigned to Rapidsilicon team
Thanks @tangxifan. You captured my thoughts well. Basically refactoring into a separate library may not be desirable with a low-level function interface (which is what we have) as we may need inlining for acceptable performance, and that will be left the linker and may not be possible or reliable.
@tangxifan which API are you referencing above called nodes()
? I could only find usages of a function called nodes()
for the TimingGraph
. The same is true for rr_switches()
. Are these meant to be brand new APIs? If so, what would be your desired functionality for each?
@ethanroj23 You are right.
nodes()
should be a new API in place of the current methods that are used to walk through all the nodesIt is designed when we consider the context of t_rr_node
(there are only nodes and we can access a node using rr_node[i]
)
However, now, this is a graph. The accessor API should be more precise. For example,
for (const RRNodeId& node : rr_graph.nodes()) {
// Use node as the id to get more attributes
}
You can refer to my implementation at
It will help you when implementing the method
For other APIs, we may consider to deprecate them. Since current APIs always relay on an id to get the information you want, we no long return a data structure t_rr_node
anymore. However, it is safer to double check. I am open to add other APIs if necessary.
For rr_switches()
, it is brand new APIs. It is a similar story than merging the rr_segments
into RRGraphView
. Can you look into the PR #1910 ? After that, if you are still confused, we can talk.
Let me know what you think.
@tangxifan Thank you for the explanation, this makes more sense now! I will begin implementation of this new API and look for feedback once I have done enough for a WIP PR.
Hi @ethanroj23 I believe you can try the vtr::make_range(begin(), end())
, since the existing APIs begin()
and end()
are already there. You can refer to the rr_graph_ohj.h
The begin()
and end()
are actually iterators. Just need to find a way to use the make_range()
function
@behzadmehmood-rs @umariqbal-rs Thank you very much on the good work. We have accomplished all the major tasks in this refactoring effort. We are approaching the final destination! I have listed the last few steps before we can extract the routing resource graph data structure as library.
Please let me know which step you want to take the ownership.
@tangxifan Thank you for your support. We are ready to own the first four tasks.
@tangxifan : I think this can be closed. Agreed?
Or maybe it's waiting for the last item: unit tests?
@vaughnbetz The unit tests have been built in #2150 but not yet merged. Feel sorry for the delayed review. I will catch that this summer. I am o.k. to close the PR and create a new one on the unit tests.
Motivation of Refactoring effort
A detailed technical plan can be found at link
The overall refactoring effort aims to
RRGraphView
as a centralized storage for all the routing resource graph -related information. See Fig. 1RRGraph
for client functions, which are suitable forrr_graph builder
,placer
,router
,GUI
,timing analyzer
etc. See Fig. 2.Fig. 1 Illustration of the relationship between data structures
Fig. 2 Different levels of frame views of routing resource graphs to satisfy various needs from client functions.
The result/benefits of the refactoring efforts is
RRGraph
. A routing resource graph builder can be a library, decoupled from VPR's core engine. Many checking codes can be efficiently merged into the data structure and developers can save a lot of efforts in writing the atom-level sanity checks.RRGraphView
. In other words, routing resource graph will be read-only and only accessors are exposed to client functions. Developers have no worries on developing their own placer/router etc.Checklist
RRGraphBuilder API to be refactored:
add_node()Removed because it is not currently used in rr_graph builder; Will think it later.RRGraphView API to be refactored: