RRGraphView node_cost_index() Implementation #1843

ethanroj23 · 2021-09-09T14:53:47Z

Description

In this PR, I have implemented RRGraphView::node_cost_index() throughout VTR. Every time device_ctx.rr_nodes[index].cost_index() was called has been replaced with rr_graph.cost_index(RRNodeId(index)). In order to do this, I followed a pattern similar to that in the last PR.

Motivation and Context

These changes are a continuation of the RRGraphView API implementation effort.

How Has This Been Tested?

I have run the regression tests for vtr_reg_strong along with the QoR testing found at $ ../scripts/run_vtr_task.py regression_tests/vtr_reg_nightly_test3/vtr_reg_qor_chain. Results from these QoR tests can be found below. The file containing all results can be found here

Only rows that are different are shown.

	VTR Before These Changes	With Changes in this PR
vtr_flow_elapsed_time	1	1.013103428
odin_synth_time	1	1.004124928
abc_synth_time	1	1.009521229
max_vpr_mem	1	0.999794275
pack_time	1	0.984221145
place_time	1	1.004760669
min_chan_width_route_time	1	1.01720273
crit_path_route_time	1	1.019740612

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

Signed-off-by: Ethan Rogers <[email protected]>

tangxifan

@ethanroj23 Looks good to me. Just minor changes required.

I actually would like to know @vaughnbetz 's and your opinion about creating a strong id for the cost index RRIndexedDataId. It will help us to avoid the misuse of the cost index.

Accordingly, you need to modify the data type of rr_indexed_data

vtr-verilog-to-routing/vpr/src/base/vpr_context.h

Line 152 in 77a3df8

std::vector<t_rr_indexed_data> rr_indexed_data; // [0 .. num_rr_indexed_data-1]

to

vtr::vector<RRIndexedDataId, t_rr_indexed_data> rr_indexed_data

You can refer to

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

Lines 16 to 24 in 77a3df8

    
           struct rr_node_id_tag; 
        
           struct rr_edge_id_tag; 
        
           struct rr_switch_id_tag; 
        
           struct rr_segment_id_tag; 
        
           typedef vtr::StrongId<rr_node_id_tag, unsigned int> RRNodeId; 
        
           typedef vtr::StrongId<rr_edge_id_tag, unsigned int> RREdgeId; 
        
           typedef vtr::StrongId<rr_switch_id_tag, short> RRSwitchId; 
        
           typedef vtr::StrongId<rr_segment_id_tag, short> RRSegmentId;

about how to create the strong id.

Note: we may still keep the short data type inside the rr_graph_storage for the sake of memory efficiency. But the API should always return a strong data type.

Let me know what you think. I am open to discussion.

tangxifan · 2021-09-10T17:27:19Z

vpr/src/route/rr_graph_uxsdcxx_serializer.h

        }
        return inode;
    }
    inline void finish_node_segment(int& /*inode*/) final {}
    inline int get_node_segment_segment_id(const t_rr_node& node) final {
-        return (*rr_indexed_data_)[node.cost_index()].seg_index;
+        return (*rr_indexed_data_)[(*rr_graph_).node_cost_index(node.id())].seg_index;


You may use rr_graph->node_cost_index to avoid too many brackets

vaughnbetz · 2021-09-10T18:50:57Z

vpr/src/route/segment_stats.cpp

@@ -50,7 +50,7 @@ void get_segment_usage_stats(std::vector<t_segment_inf>& segment_inf) {

    for (size_t inode = 0; inode < device_ctx.rr_nodes.size(); inode++) {
        if (rr_graph.node_type(RRNodeId(inode)) == CHANX || rr_graph.node_type(RRNodeId(inode)) == CHANY) {
-            cost_index = device_ctx.rr_nodes[inode].cost_index();
+            cost_index = rr_graph.node_cost_index(RRNodeId(inode));


Here's an example of where I think we should eventually hide the cost_index.
Instead of asking for a cost_index, and then looking up seg_type, we instead should have an api that takes an RRNodeId and returns the seg_type
seg_type = rr_graph.seg_type(RRNodeId(inode));
Internally, the .seg_type() function would get the cost_index, and then look up the seg_type and return it.

It hides the fact that this seg_type lookup goes through a cost_index (flyweight) for storage efficiency; the caller doesn't care.

It's possible there's some cpu time penalty to that in some code, if we ask for a bunch of cost_index data in rapid succession. But in that case we could return a struct with the data that tends to be requested together packed into it.

@vaughnbetz I think it is a good idea to streamline the API here. Actually, I am thinking about if we should include the rr_segments and rr_switch_inf as part of the RRGraphView and RRGraphBuilder.

vtr-verilog-to-routing/vpr/src/base/vpr_context.h

Lines 173 to 177 in 77a3df8

///@brief Autogenerated in build_rr_graph based on switch fan-in. [0..(num_rr_switches-1)]

std::vector<t_rr_switch_inf> rr_switch_inf;

///@brief Wire segment types in RR graph

std::vector<t_segment_inf> rr_segments;

I said so because these two data structures are modified copies of the original ones in architecture data. They have been updated to accurately model the switches and segments in the context of a specific routing resource graph.
Since they are adhoc to RRGraph, they can be part of the graph and we can provide shortcut APIs rather than force developers to visit several data structures.

However, it may be too complicated to this PR. It can be in a pull request near future (I can take the lead on it; since I have already tried in the rr_graph_obj.h)

Let me know what you think.

If we decide to add API calls, the following are all a part of t_rr_indexed_data: base_cost, saved_base_cost, ortho_cost_index, seg_index, inv_length, T_linear, T_quadratic, C_load.

The ortho_cost_index is used to access each of these data points for a different cost index, so we would have to consider how the API would handle that. We could have a second argument to the functions rr_graph.seg_type(rr_node, is_ortho) or potentially two separate functions rr_graph.seg_type(), rr_graph.ortho_seg_type()

I agree, I think both those data structures should go in RRGraphView, and be built in RRGraphBuilder.
I also talked to @kmurray about this today in another meeting, and he also thinks hiding the cost_index and rr_indexed_data in RRGraphView api calls is the right way to go.

I think the rr_graph.ortho_seg_type() style would be more clear.
For items that tend to be used together, I'd make the API return a tuple or struct with them packed in (e.g. inv_length, T_linear, T_quadratic and C_load are all probably used just for the classic lookahead and could be returned as a group only).

vaughnbetz · 2021-09-10T18:52:56Z

I like the idea of returning a StrongId, if we keep the api to return a cost_index. It should be stored as a short internally though.

Eventually I think the rr-indexed accesses should all be wrapped in an api presented by RRGraphView so that the various data we store using cost_index in a flyweight pattern (store the unique values) is just requested from RRGraphView. See an example in the comment above; there would be a few functions like that.

So the question is: should we wrap cost_index in an api now, or just skip that step and wrap the rr_indexed data accesses in an api, and hide cost_index? Either way, we should eventually get to the rr_indexed data accesses being wrapped and directly accessed using RRGraphView.some_func(inode) .

vaughnbetz · 2021-09-13T16:16:32Z

So the summary:

cost_index: use a StrongId
should hide cost index behind new methods in RRGraphView that let us directly return the data stored on a cost_index (hide the flyweight).
should move rr_segments and rr_switch_inf into the RRGraphView/RRGraphBuilder eventually.

Should #1 be done in this PR or later?
I believe #2 should be done in separate PRs after this one, and the same for #3.
@ethanroj23 : what is your preference?

tangxifan · 2021-09-13T23:55:52Z

It seems that ODIN-II basic tests are stuck constantly (at least on my pull request #1847 and this one). Not sure what the issue is. It is a required test. As a result, it may block pull request merging.

vaughnbetz · 2021-09-14T00:15:51Z

Yes, this is consistently stuck. I can override and merge.
But we should probably resolve the questions above first: merge as is, or implement 1, 2, or 3 first?

vaughnbetz · 2021-09-14T00:18:32Z

@sdamghan : it seems the odinII basic tests are still showing up in recent PRs? Is there some config update that needs to happen?

tangxifan · 2021-09-14T00:19:24Z

Yes, this is consistently stuck. I can override and merge.
But we should probably resolve the questions above first: merge as is, or implement 1, 2, or 3 first?

I would like to hear from @ethanroj23 which way he prefer.
I can help whenever he needs.
My view is

Option 1 is doable for this PR (it can be in a separated PR if we prefer single objective per PR).
Option 2,3 can be in later PRs.

sdamghan · 2021-09-14T00:43:59Z

@sdamghan : it seems the odinII basic tests are still showing up in recent PRs? Is there some config update that needs to happen?

@vaughnbetz
I do not think there is anything more than what we should specify in the VTR_ROOT/.github/workflows/test.yml file to determine the GitHub actions, however, I'll again investigate this.
I think I need to specify here that ODIN-II Basic Tests is deprecated and no longer performs any CI test. I mean it is only an empty label. So until I find a solution to remove it from the CI list please feel free to ignore it. I should mention that if Odin-II fails it would appear in one of the following CI tests:

GitHub Actions:

Test / ODIN-II Basic Test link
Test / Yosys+ODIN-II Basic link

Kokoro Tests:

ODIN-II Strong Tests link
Yosys+ODIN-II Strong Tests link
Yosys+Odin link

ethanroj23 · 2021-09-14T01:47:11Z

@tangxifan I think option 1 is doable as well. I will take a look tomorrow and let you know if I will need any assistance. Thanks!

sdamghan · 2021-09-14T01:52:19Z

@vaughnbetz ODIN-II Basic Tests has been hardcoded into the master branch protection rules as a required status. I think from now on, everything should be good since there is no deprecated CI label.

vaughnbetz · 2021-09-14T15:20:39Z

@sdamghan : seems like it now shows up as finished, and has a detailed result, but no "required".
We can talk about this on Thursday. If it is not needed (deprecated) it seems like it should be possible to remove it from the CI list.
The master branch protection rules say all basic tests must pass, but I don't see anything that refers to Odin-II basic tests specifically (but I assume they match as a basic test).

Signed-off-by: Ethan Rogers <[email protected]>

sdamghan · 2021-09-14T16:19:11Z

@vaughnbetz ODIN-II Basic Tests was showing in the CI list since it was mentioned in master branch rules. Since ODIN-II Basic Tests was replaced with Test/ODIN-II Basic Test, it was continuously running and the status was pending. I removed ODIN-II Basic Tests from branch rules last night. You are right, I forgot to label the Test/ODIN-II Basic Test and Test/Yosys+Odin-II basic tests as required tests. I have just labelled them. I believe Odin-II and Yosys+Odin-II CI tests should be fine now. I will provide more information on Thursday if required.

Signed-off-by: Ethan Rogers <[email protected]>

vaughnbetz · 2021-09-17T17:38:12Z

vpr/src/route/rr_graph_storage.h

+        return RRIndexedDataId(node_storage_[id].cost_index_);
+    }
+
+    bool cost_index_is_invalid(RRNodeId id) const {


As discussed in the VTR meeting, we can get rid of this and use the StrongID::Invalid() and/or it's bool test functionality.

Signed-off-by: Ethan Rogers <[email protected]>

vaughnbetz · 2021-09-28T20:42:59Z

Code changes look good, but there are conflicts to resolve @ethanroj23 . Can you resolve them and then we can merge?

Signed-off-by: Ethan Rogers <[email protected]>

ethanroj23 · 2021-09-29T13:50:23Z

Code changes look good, but there are conflicts to resolve @ethanroj23 . Can you resolve them and then we can merge?

Conflicts are now resolved. We should be good to go.

tangxifan · 2021-10-11T20:32:40Z

Link to #1868 So that we keep tracking.

ethanroj23 added 2 commits September 7, 2021 09:39

Initial implementation of node_cost_index() in RRGraphView

29ef568

Signed-off-by: Ethan Rogers <[email protected]>

Merge remote-tracking branch 'vtr/master' into rr_graph_node_cost_index

c2ee1cb

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Sep 9, 2021

Removed unused variable

46a1f6a

Signed-off-by: Ethan Rogers <[email protected]>

ethanroj23 requested review from hzeller, tangxifan and vaughnbetz September 10, 2021 15:01

tangxifan requested changes Sep 10, 2021

View reviewed changes

vaughnbetz reviewed Sep 10, 2021

View reviewed changes

ethanroj23 added 2 commits September 14, 2021 09:40

Added strong id RRIndexedDataId

57c333b

Signed-off-by: Ethan Rogers <[email protected]>

Formatting fixes

75018b5

Signed-off-by: Ethan Rogers <[email protected]>

ethanroj23 added 3 commits September 14, 2021 11:17

Added cost_index_is_invalid() to check if RRIndexedDataId < 0

b75944f

Signed-off-by: Ethan Rogers <[email protected]>

Change cast from (int) to (size_t) to remove warning.

32323d1

Signed-off-by: Ethan Rogers <[email protected]>

Removed unnecessary function cost_index_is_invalid

dcdc95f

Signed-off-by: Ethan Rogers <[email protected]>

vaughnbetz reviewed Sep 17, 2021

View reviewed changes

Removed unused cost_index_is_invalid function

8c1d49c

Signed-off-by: Ethan Rogers <[email protected]>

tangxifan mentioned this pull request Sep 27, 2021

[CI] Logs Memory during CI Run #1851

Merged

7 tasks

Merge remote-tracking branch 'vtr/master' into rr_graph_node_cost_index

e1058cb

Signed-off-by: Ethan Rogers <[email protected]>

vaughnbetz merged commit 7554512 into verilog-to-routing:master Sep 29, 2021

tangxifan mentioned this pull request Oct 14, 2021

Dashboard for RRGraph API refactoring status #1868

Open

44 tasks

	struct rr_node_id_tag;
	struct rr_edge_id_tag;
	struct rr_switch_id_tag;
	struct rr_segment_id_tag;

	typedef vtr::StrongId<rr_node_id_tag, unsigned int> RRNodeId;
	typedef vtr::StrongId<rr_edge_id_tag, unsigned int> RREdgeId;
	typedef vtr::StrongId<rr_switch_id_tag, short> RRSwitchId;
	typedef vtr::StrongId<rr_segment_id_tag, short> RRSegmentId;

	///@brief Autogenerated in build_rr_graph based on switch fan-in. [0..(num_rr_switches-1)]
	std::vector<t_rr_switch_inf> rr_switch_inf;

	///@brief Wire segment types in RR graph
	std::vector<t_segment_inf> rr_segments;

RRGraphView node_cost_index() Implementation #1843

RRGraphView node_cost_index() Implementation #1843

Uh oh!

Conversation

ethanroj23 commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

tangxifan left a comment • 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!

vaughnbetz commented Sep 10, 2021

Uh oh!

vaughnbetz commented Sep 13, 2021

Uh oh!

tangxifan commented Sep 13, 2021

Uh oh!

vaughnbetz commented Sep 14, 2021

Uh oh!

vaughnbetz commented Sep 14, 2021

Uh oh!

tangxifan commented Sep 14, 2021

Uh oh!

sdamghan commented Sep 14, 2021

GitHub Actions:

Kokoro Tests:

Uh oh!

ethanroj23 commented Sep 14, 2021

Uh oh!

sdamghan commented Sep 14, 2021

Uh oh!

vaughnbetz commented Sep 14, 2021

Uh oh!

sdamghan commented Sep 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaughnbetz commented Sep 28, 2021

Uh oh!

ethanroj23 commented Sep 29, 2021

Uh oh!

tangxifan commented Oct 11, 2021

Uh oh!

Uh oh!

ethanroj23 commented Sep 9, 2021 •

edited

Loading

tangxifan left a comment •

edited

Loading