Remove legacy data structure `rr_node_indices` from `DeviceContext` #1828

tangxifan · 2021-08-16T01:32:40Z

Description

This PR focuses on updating routing resource graph builder functions, where we use the refactored data structure RRGraphBuilder to replace the legacy data structure rr_node_indices.
This PR aims to fully deprecate the direct use of the legacy data structure rr_node_indices in builder as well as client functions.

After this PR, the rr_node_indices data structure is fully shadowed and encapsulated in the refactored data structure RRGraphBuilder.

This is the first milestone achieved in the refactoring effort on routing resource graph data structures.

Checklist:

Removed the legacy data structure rr_node_indices from DeviceContext
Reworked the function verify_rr_node_indices() to use RRGraphBuilder rather than rr_node_indices
Added new APIs clear() to data structures RRGraphBuilder and RRSpatialLookup to reset the data storage; As a result, the free_rr_graph() is reworked to use the new API.
Added a new API reorder() to RRSpatialLookup to support graph reordering. As a result, the reordering algorithm is reworked to use the new API.

TODO: May consider to implement the API reorder() in RRGraphBuilder so that the current reordering function can be fully internal.

Related Issue

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

Motivation and Context

After the previous PR #1805 , we start reworking all the source files that use the legacy data structure rr_node_indices in a high priority, in order to deprecate the legacy data structure as soon as possible.
Current statistics on the files that use rr_node_indices:

./route/rr_graph2.cpp
./route/rr_graph2.h
./base/vpr_context.h

This PR will remove the use in
./route/rr_graph.cpp
./route/rr_graph2.cpp
./base/vpr_context.h

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

… in place of the legacy data structure ``rr_node_indices``

…eContext, as it is now fully shadowed by the data structure ``RRSpatialLookup``

tangxifan · 2021-08-20T16:18:51Z

As required for QoR checks, I finished the QoR runs on the

Basic sanity test
- peak memory usage +2% on average
vtr_reg_qor_chain_depop test
- flow run time +2% on average. However, most contributions are from ODIN2 (+23%) and packer (+2%), which are not under refactoring
- No changes on critical path delay and peak memory usage
titan_quick_qor test:
- flow run time +4% on average. However, most contributions are from packer (+3%), which are not under refactoring
- No changes on critical path delay
- Peak memory usage -1% on average

Attached spreadsheet for the detailed QoR of each test:

vtr_reg_basic_timing_sanity_comp.xlsx
vtr_reg_titan_quick_qor_comp.xlsx
vtr_reg_qor_chain_depop_comp.xlsx

tangxifan · 2021-08-20T21:35:25Z

@vaughnbetz @hzeller This pull request is ready for your review. QoR results updated and CI is green.

hzeller

LGTM

hzeller · 2021-08-27T05:12:14Z

vpr/src/device/rr_spatial_lookup.h

@@ -17,7 +18,7 @@ class RRSpatialLookup {
    /* -- Constructors -- */
  public:
    /* Explicitly define the only way to create an object */
-    explicit RRSpatialLookup(t_rr_node_indices& rr_node_indices);
+    explicit RRSpatialLookup();


Since we don't take a parameter anymore, the explicit is not needed anymore.
The explicit is only needed to tell the compiler to not consider this constructor in an automatic type-conversion, but if there are zero parameters, that is not an issue anymore.

Make sense to me. Will remove it.

hzeller · 2021-08-27T05:13:05Z

vpr/src/device/rr_spatial_lookup.cpp

@@ -273,3 +272,26 @@ void RRSpatialLookup::resize_nodes(int x,
                                       std::max(rr_node_indices_[type].dim_size(2), size_t(side) + 1)});
    }
 }
+
+void RRSpatialLookup::reorder(const vtr::vector<RRNodeId, RRNodeId> dest_order) {


I'd expect some sort of unit-test for this method ?
(I know it is moved from some other place without change, so ok not adding a test for now; but now, it is actually a nicely testable functionality)

You are right. This feature is already under testing in one of the test case in vtr_reg_basic:

vtr-verilog-to-routing/vtr_flow/tasks/regression_tests/vtr_reg_basic/basic_timing/config/config.txt

Line 32 in 29a9fd5

script_params_list_add = --reorder_rr_graph_nodes_algorithm random_shuffle

I am not sure if this can be categorized as a unit-test. If not, we should think about it once we can extract the RRGraphBuilder and RRGraphView as an independent library.
Let me know what you think. I think unit test is a good idea.

vaughnbetz

Looks good to me. A few suggestions for commenting improvements, which could be done after merge if more convenient then.

vaughnbetz · 2021-08-27T21:31:15Z

vpr/src/device/rr_graph_builder.h

@@ -63,7 +65,7 @@ class RRGraphBuilder {
    /* node-level storage including edge storages */
    t_rr_graph_storage& node_storage_;
    /* Fast look-up for rr nodes */
-    RRSpatialLookup& node_lookup_;
+    RRSpatialLookup node_lookup_;
 };



Now that we have the RRGraphView and RRGraphBuilder APIs, we should make doxygen documentation for them. Best done as a follow-up issue, so I'll file an issue for that (make doxygen-compatible comments, and add to the VPR API on the read-the-docs page).

vaughnbetz · 2021-08-27T21:34:51Z

vpr/src/device/rr_spatial_lookup.h

-     * If pointers are used, it may cause many codes in client functions 
-     * or inside the data structures to be changed later.
-     * That explains why the reference is used here temporarily
-     */
    /* Fast look-up: TODO: Should rework the data type. Currently it is based on a 3-dimensional arrqay mater where some dimensions must always be accessed with a specific index. Such limitation should be overcome */


typo to fix: 3-dimensional array matrix

vaughnbetz · 2021-08-27T21:43:04Z

Merging since CI is green, but please see the commenting suggestions above (typo and doxygen) for a future PR.

tangxifan · 2021-08-27T21:46:10Z

Thanks @vaughnbetz I will create a follow-up pull request to address your comment today.

tangxifan added 2 commits August 15, 2021 18:03

[VPR] Reworked verify_rr_node_indices function to use RRSpatialLookup…

ba0eebd

… in place of the legacy data structure ``rr_node_indices``

[VPR] Remove the legacy data structure rr_node_indices from Devic…

9117490

…eContext, as it is now fully shadowed by the data structure ``RRSpatialLookup``

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Aug 16, 2021

[VPR] Code format fix

a5217ec

tangxifan added the kokoro:force-run label Aug 20, 2021

symbiflow-robot removed the kokoro:force-run label Aug 20, 2021

tangxifan requested review from vaughnbetz and hzeller August 20, 2021 21:35

tangxifan mentioned this pull request Aug 24, 2021

Yosys+Odin #1798

Merged

7 tasks

hzeller approved these changes Aug 27, 2021

View reviewed changes

tangxifan removed the VPR VPR FPGA Placement & Routing Tool label Aug 27, 2021

vaughnbetz reviewed Aug 27, 2021

View reviewed changes

vaughnbetz merged commit 5eb14aa into master Aug 27, 2021

vaughnbetz deleted the remove_rr_node_indices branch August 27, 2021 21:43

tangxifan mentioned this pull request Aug 31, 2021

Add RRGraph-related API to documentation and clean-up RRSpatialLookup methods #1837

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove legacy data structure `rr_node_indices` from `DeviceContext` #1828

Remove legacy data structure `rr_node_indices` from `DeviceContext` #1828

Uh oh!

tangxifan commented Aug 16, 2021 •

edited

Loading

Uh oh!

tangxifan commented Aug 20, 2021 •

edited

Loading

Uh oh!

tangxifan commented Aug 20, 2021

Uh oh!

hzeller left a comment

Uh oh!

hzeller Aug 27, 2021

Uh oh!

tangxifan Aug 27, 2021

Uh oh!

hzeller Aug 27, 2021

Uh oh!

tangxifan Aug 27, 2021

Uh oh!

vaughnbetz left a comment

Uh oh!

vaughnbetz Aug 27, 2021

Uh oh!

vaughnbetz Aug 27, 2021

Uh oh!

vaughnbetz commented Aug 27, 2021

Uh oh!

tangxifan commented Aug 27, 2021

Uh oh!

Uh oh!

Remove legacy data structure rr_node_indices from DeviceContext #1828

Remove legacy data structure rr_node_indices from DeviceContext #1828

Uh oh!

Conversation

tangxifan commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

tangxifan commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tangxifan commented Aug 20, 2021

Uh oh!

hzeller left a comment

Choose a reason for hiding this comment

Uh oh!

hzeller Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

tangxifan Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

hzeller Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

tangxifan Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

vaughnbetz left a comment

Choose a reason for hiding this comment

Uh oh!

vaughnbetz Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

vaughnbetz Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

vaughnbetz commented Aug 27, 2021

Uh oh!

tangxifan commented Aug 27, 2021

Uh oh!

Uh oh!

Remove legacy data structure `rr_node_indices` from `DeviceContext` #1828

Remove legacy data structure `rr_node_indices` from `DeviceContext` #1828

tangxifan commented Aug 16, 2021 •

edited

Loading

tangxifan commented Aug 20, 2021 •

edited

Loading