-
Notifications
You must be signed in to change notification settings - Fork 415
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
RRGraphView node_cost_index() Implementation #1843
Conversation
Signed-off-by: Ethan Rogers <[email protected]>
There was a problem hiding this 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
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.
} | ||
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; |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
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.
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
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) . |
So the summary:
Should #1 be done in this PR or later? |
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. |
Yes, this is consistently stuck. I can override and merge. |
@sdamghan : it seems the odinII basic tests are still showing up in recent PRs? Is there some config update that needs to happen? |
I would like to hear from @ethanroj23 which way he prefer.
|
@vaughnbetz GitHub Actions:Kokoro Tests: |
@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! |
@vaughnbetz |
@sdamghan : seems like it now shows up as finished, and has a detailed result, but no "required". |
Signed-off-by: Ethan Rogers <[email protected]>
Signed-off-by: Ethan Rogers <[email protected]>
@vaughnbetz |
Signed-off-by: Ethan Rogers <[email protected]>
Signed-off-by: Ethan Rogers <[email protected]>
Signed-off-by: Ethan Rogers <[email protected]>
vpr/src/route/rr_graph_storage.h
Outdated
return RRIndexedDataId(node_storage_[id].cost_index_); | ||
} | ||
|
||
bool cost_index_is_invalid(RRNodeId id) const { |
There was a problem hiding this comment.
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.
Signed-off-by: Ethan Rogers <[email protected]>
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]>
Conflicts are now resolved. We should be good to go. |
Link to #1868 So that we keep tracking. |
Description
In this PR, I have implemented
RRGraphView::node_cost_index()
throughout VTR. Every timedevice_ctx.rr_nodes[index].cost_index()
was called has been replaced withrr_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 hereOnly rows that are different are shown.
Types of changes
Checklist: