API reorder_rr_graph_node() created in rr_graph_util.cpp #1939

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.

Sign up for GitHub

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

Jump to bottom

Merged

tangxifan merged 8 commits into verilog-to-routing:master from RapidSilicon:reorder_rr_graph_nodes_function_position_change

Jan 6, 2022

Contributor

Raza-jafari-RS commented Dec 16, 2021

Description

In this PR, I have relocated the reorder_rr_graph_node() from rr_graph_util.cpp to rr_graph_builder.cpp.

Related Issue

Motivation and Context

As it was devised in the node_first_edge() Pull Request discussion.

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


           after adding api

0cfed30

github-actions bot added the VPR label

behzadmehmood-rs assigned Raza-jafari-RS


          Completely removing the function from rr_graph_util

17f164c

Contributor Author

Raza-jafari-RS commented Dec 19, 2021 •

edited

Loading

@tangxifan
I have replaced the function reorder_rr_graph_nodes() from rr_graph_util.cpp to rr_graph_builder.cpp as below.
https://github.com/RapidSilicon/vtr-verilog-to-routing/blob/17f164c5350aed7c460855e662f5be68bb67bf4f/vpr/src/device/rr_graph_builder.cpp#L57
And I made changes in respective files.
Can you please review them and suggest me how to move forward.

tangxifan requested changes

View reviewed changes

Contributor

tangxifan left a comment

@Raza-jafari-RS Let me know if you are fine with the comments. Feel free to ask questions.

vpr/src/device/rr_graph_builder.cpp Outdated

@@ @@ -1,5 +1,8 @@ @@
               #include "vtr_log.h"
               #include "rr_graph_builder.h"
+              #include "vtr_time.h"
+              #include <queue>
+              #include "globals.h"

Contributor

tangxifan Dec 19, 2021

No globals in the rr_graph_builder. The data structure should be self-contained.

vpr/src/device/rr_graph_builder.cpp Outdated

+              void RRGraphBuilder::reorder_rr_graph_nodes(const t_router_opts& router_opts) {
+                  auto& device_ctx = g_vpr_ctx.mutable_device();
+                  auto& rr_graph = device_ctx.rr_graph;

Contributor

tangxifan Dec 19, 2021

Now node_storage_ is already an internal data of the RRGraphBuilder. You do not need to call global variables anymore.

vpr/src/device/rr_graph_builder.cpp Outdated

+                  auto& rr_graph = device_ctx.rr_graph;
+                  size_t v_num = node_storage_.size();
+                  if (router_opts.reorder_rr_graph_nodes_algorithm == DONT_REORDER) return;

Contributor

tangxifan Dec 19, 2021

This if-condition can be done outside this API.
When this API is called, it will definitely do the reordering.

In client functions, we can call the API in this way:

if (router_opts.reorder_rr_graph_nodes_algorithm != DONT_REORDER) {
  rr_graph_builder.reorder_nodes();
}

vpr/src/device/rr_graph_builder.cpp Outdated

@@ @@ -50,3 +53,75 @@ void RRGraphBuilder::add_node_to_all_locs(RRNodeId node) { @@
               void RRGraphBuilder::clear() {
                   node_lookup_.clear();
               }
+              void RRGraphBuilder::reorder_rr_graph_nodes(const t_router_opts& router_opts) {

Contributor

tangxifan Dec 19, 2021

You may rename the API to be reorder_nodes(), which is short and clean.
Instead of using t_router_opts, the input argument can be the enumeration of the reorder_rr_graph_nodes_algorithm. We always avoid to include an overkill data structure as an input arguement.

vpr/src/device/rr_graph_builder.cpp Outdated

+                              RRNodeId u = que.front();
+                              que.pop();
+                              degree[u] += node_storage_.num_edges(u);
+                              for (RREdgeId edge = rr_graph.node_first_edge(u); edge < rr_graph.node_last_edge(u); edge = RREdgeId(size_t(edge) + 1)) {

Contributor

tangxifan Dec 19, 2021

node_first_edge() and node_last_edge() are APIs of node_storage_. We do not need to call rr_graph here.

vpr/src/device/rr_graph_builder.cpp Outdated


		node_lookup().reorder(dest_order);

		device_ctx.rr_node_metadata.remap_keys([&](int node) { return size_t(dest_order[RRNodeId(node)]); });

Contributor

tangxifan Dec 19, 2021

We can do the reordering node/edge metadata outside the RRGraphBuilder, since they are not internal data here.
The dest_order is the final ordering. @ethanroj23 is already doing the API in his PR #1932
Once his PR is in, you can implement it in this way.

vpr/src/device/rr_graph_builder.h Outdated

@@ @@ -59,6 +59,20 @@ class RRGraphBuilder { @@
                   /** @brief Clear all the underlying data storage */
                   void clear();
+                  // Reorder RRNodeId's using one of these algorithms:

Contributor

tangxifan Dec 19, 2021

Please follow the comment style of other APIs, which are compatible with Doxygen.
As such, the API comments will be shown in online documentation.

Raza Jafari added 3 commits

December 20, 2021 15:21


          (All done except one)

0d9b9dd


           The changes recemended mainy the remove of globals.h and function ar…

96f351b

…guments passing)


          (removing the structure used and adding the globals back)

d2a3d1a

Contributor Author

Raza-jafari-RS commented Dec 22, 2021

@tangxifan
In my view all other issues has been resolved except removing the globals as the rr_node_metadata and the rr_edge metadata can not be moved outside the function the reason behind this is that they were using the internal data (v_num) in it.
please guide me about how to move them out of this function.
Moreover the API's node_first_edge and the node_last_edge are now useless as they were not called at any location. Should I remove them or keep them for future use?

Contributor

tangxifan commented Dec 22, 2021

@Raza-jafari-RS

size_t v_num = node_storage_.size();

This can be achieved by calling existing API

device_ctx.rr_nodes.size()

When @ethanroj23 's PR #1932 is in, we can use the new API from RRGraphView.

Contributor Author

Raza-jafari-RS commented Dec 22, 2021

@tangxifan
In my view all other issues has been resolved except removing the globals as the rr_node_metadata and the rr_edge metadata can not be moved outside the function the reason behind this is that they were using the internal data (v_num) in it.
please guide me about how to move them out of this function.
Moreover the API's node_first_edge and the node_last_edge are now useless as they were not called at any location. Should I remove them or keep them for future use?

@Raza-jafari-RS
size_t v_num = node_storage_.size();
This can be achieved by calling existing API
device_ctx.rr_nodes.size()
When @ethanroj23 's PR #1932 is in, we can use the new API from RRGraphView.

Okay I am doing using the device_ctx.rr_nodes.size() and once the PR #1932 is complete I will change it accordingly

Raza Jafari added 2 commits

December 22, 2021 16:46


          (removing the globals from rr_graph_builder.cpp)

72a35cb


          (globals removed from rr_graph_builder)

Contributor Author

Raza-jafari-RS commented Dec 22, 2021

@tangxifan,
The globals has been removed from the function but the rr_node_metadata and rr_edge_metadata are not moved out of this function as the basic c test fails after moving out.
please review the commit and guide me about how to move further.

Contributor

tangxifan commented Dec 22, 2021

The changes make sense to me. Good work @Raza-jafari-RS
Can you provide QoR tests?

Contributor

umariqbal-rs commented Dec 29, 2021 •

edited

Loading

@tangxifan
QoR tests are complete and quick summary is given below. QoR comparison files are also attached.
vtr_reg_qor_chain_depop test
Flow run time -0.46% on average.
critical path delay 0% on average.
Peak memory usage 0.04% on average
titan_quick_qor test:
flow run time 2.68% 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

Raza-jafari-RS marked this pull request as ready for review

December 29, 2021 08:10

Raza-jafari-RS requested a review from tangxifan

December 29, 2021 08:55

Contributor

tangxifan commented Dec 29, 2021

@Raza-jafari-RS @umariqbal-rs Thanks for the updates. QoR tests look good.

@vaughnbetz I propose to merge. However, I believe it would be great that you can take a quick review on this PR, before I merge. The main point is to see if you agree on

If we should move the rr_node_metadata and rr_edge_metadata into the RRGraphBuilder
It will not break any backward compatibility. If you know any authors of the rr_node_metadata and rr_edge_metadata or who will be impacted (@mithro Perhaps symbiflow will be impacted?), feel free to ask those guys for a review.

Also, I believe it would be great to add more comments for this API reorder_nodes, if you know anyone can help.
We appreciate your help and support on this effort.

vaughnbetz reviewed

View reviewed changes

vpr/src/device/rr_graph_builder.h Show resolved Hide resolved

vaughnbetz reviewed

View reviewed changes

vpr/src/device/rr_graph_builder.h Show resolved Hide resolved

vaughnbetz reviewed

View reviewed changes

vpr/src/route/rr_graph_util.h Outdated Show resolved Hide resolved

Contributor

vaughnbetz commented Dec 31, 2021

Moving a reference to the metadata into the RRGraphBuilder makes sense to me, as you've done so far.

@tangxifan: are you proposing to make RRGraphBuilder the owner of the underlying storage too (i.e. move the data, not just a reference, into the RRGraphBuilder)? That might make sense but I haven't looked at the code in detail. @acomodi might know more.

@Raza-jafari-RS : I've added some proposed comments giving high-level context on rr-graph reordering (why would it be used?) and metadata. Please incorporate those in the final PR.

Contributor

tangxifan commented Dec 31, 2021

@vaughnbetz Thanks a lot on the comments. Happy new year by the way.

This PR has not done so actually. We are still using references in RRGraphBuilder, which means DeviceContext is still the owner of the metadata. I raise this question because I believe it will impact our future PRs.

Regarding the final owner of the metadata, I think that RRGraphBuilder can be the owner because we agree that the reordering is an internal business of routing resource graph. We can build APIs to return metadata just as we did for the spatial look-up.

I do not have a very strong opinion here. If @acomodi can provide more insights, we can discuss.

Contributor

vaughnbetz commented Dec 31, 2021

Happy New Year to you too Xifan!
I have no objection to moving the metadata into the RRGraphBuilder. It seems more logical than leaving it in the DeviceContext, but I haven't looked in detail.

Contributor Author

Raza-jafari-RS commented Jan 3, 2022 •

edited

Loading

@tangxifan,
Happy New Year to every one and thanks for the wishes.
I am adding the comments and I am curious about the two API's node_first_edge() and node_last_edge() as now they are not in use so should I remove them or keep them for future use?

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

Lines 128 to 135 in 1333ba7

    
           inline RREdgeId node_first_edge(RRNodeId node) const { 
        
               return node_storage_.first_edge(node); 
        
           } 
        
           /** @brief Get the last out coming edge of resource node. This function is inlined for runtime optimization. */ 
        
           inline RREdgeId node_last_edge(RRNodeId node) const { 
        
               return node_storage_.last_edge(node); 
        
           }


           Comments added

16e3a13

Contributor

tangxifan commented Jan 3, 2022

@Raza-jafari-RS Happy new year.
Please do not touch the APIs in this PR. We will review the APIs one by one once the refactoring is done.

Contributor Author

Raza-jafari-RS commented Jan 3, 2022

@tangxifan
Thanks for the wishes.
I have added the comments and also removed the unwanted lines. please review it

Contributor

tangxifan commented Jan 3, 2022

@Raza-jafari-RS Thanks for the effort. You have applied @vaughnbetz 's comments well. I will wait for @mithro 's inputs before approve on the PR.

Collaborator

acomodi commented Jan 5, 2022

@vaughnbetz @tangxifan Sorry for the late response, anyhow, I have looked a bit through the code, as I wasn't too familiar with rr metadata code as well, and I think that moving the metadata info in the RR graph builder should not cause any troubles. There also is a C++ test in the catch2 framework that should verify that the RR metadata is properly working, and it passed in this PR.

For clarity though it might be good to move the following lines just below rr_nodes: https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/master/vpr/src/base/vpr_context.h#L198-L215

Contributor

vaughnbetz commented Jan 5, 2022

Thanks @acomodi and I agree with you on the comment; we'll want to keep that so it should move if the owner of this data moves.

Contributor

tangxifan commented Jan 5, 2022

@vaughnbetz @acomodi
Thanks for the inputs. In follow-up PRs,

We will move the comment along with the data structure under the RRGraphBuilder (the owner of the data).
We will also keep an eye on the C++ unit test so that it will not break any backward compatibility.

Let me know if I misunderstand the message.

If we do not have any concerns, I suggest to merge this PR as is.

Contributor

tangxifan commented Jan 6, 2022

As discussed today with @mithro , one thing we should keep in mind on the rr_node_metadata:

It should be kept separated inside the RRGraphBuilder from other data structures, i.e., node_storage, node_lookup, RRSegment and RRSwitch

We should also add code comments (warnings) to RRGraphBuilder, so that developers will not merge the metadata into other data structures.

tangxifan reviewed

View reviewed changes

Contributor

tangxifan left a comment

@Raza-jafari-RS Good work. We decide to merge this PR.
We need a follow-up pull request to move the data into RRGraphBuilder. Some rules have to respected. Please check the code comments.

tangxifan approved these changes

View reviewed changes

tangxifan merged commit 311c460 into verilog-to-routing:master

tangxifan deleted the reorder_rr_graph_nodes_function_position_change branch

January 6, 2022 19:38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

VPR