Skip to content

RRGraphView node_cost_index() Implementation #1843

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

Merged

Conversation

ethanroj23
Copy link
Contributor

@ethanroj23 ethanroj23 commented Sep 9, 2021

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

@github-actions github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Sep 9, 2021
Signed-off-by: Ethan Rogers <[email protected]>
Copy link
Contributor

@tangxifan tangxifan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

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

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.

}
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

///@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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
Copy link
Contributor

So the summary:

  1. cost_index: use a StrongId
  2. should hide cost index behind new methods in RRGraphView that let us directly return the data stored on a cost_index (hide the flyweight).
  3. 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@tangxifan
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Member

@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.

return RRIndexedDataId(node_storage_[id].cost_index_);
}

bool cost_index_is_invalid(RRNodeId id) const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@tangxifan tangxifan mentioned this pull request Sep 27, 2021
7 tasks
@vaughnbetz
Copy link
Contributor

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

@ethanroj23
Copy link
Contributor Author

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.

@vaughnbetz vaughnbetz merged commit 7554512 into verilog-to-routing:master Sep 29, 2021
@tangxifan
Copy link
Contributor

Link to #1868 So that we keep tracking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VPR VPR FPGA Placement & Routing Tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants