RRGraphView node_R(), node_C(), node_rc_index() Implementation

[ethanroj23]

Description

In this PR, I have implemented RRGraphView::node_R(), RRGraphView::node_C() and RRGraphView::node_rc_index() throughout VTR. Every time device_ctx.rr_nodes[index].R() was called has been replaced with rr_graph.node_R(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. AFter the changes to connection_router.h, results are here

Only rows that are different are shown.

	VTR Before These Changes	With Changes in this PR	After adding RRGraphView* to connection_router.h
vtr_flow_elapsed_time	1	0.994093231	1.006881
odin_synth_time	1	1.028283499	1.01006
abc_synth_time	1	1.00525759	1.018728
max_vpr_mem	1	0.999672195	0.999557
pack_time	1	1.016018952	0.994262
place_time	1	0.996246966	0.99846
min_chan_width_route_time	1	0.995872138	1.000367
crit_path_route_time	1	1.001645744	1.009915

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

[tangxifan]

@ethanroj23 It looks good to me in general. Just a few comments to be address for a clean code organization.

[tangxifan]

The connection_router object actually caches the rr_graph, in order to avoid the use of global variables. I suggest to add the RRGraphView to the cache here:

vtr-verilog-to-routing/vpr/src/route/connection_router.h

Lines 27 to 44 in 2e0bafe

	ConnectionRouter(
	const DeviceGrid& grid,
	const RouterLookahead& router_lookahead,
	const t_rr_graph_storage& rr_nodes,
	const std::vector<t_rr_rc_data>& rr_rc_data,
	const std::vector<t_rr_switch_inf>& rr_switch_inf,
	std::vector<t_rr_node_route_inf>& rr_node_route_inf)
	: grid_(grid)
	, router_lookahead_(router_lookahead)
	, rr_nodes_(rr_nodes.view())
	, rr_rc_data_(rr_rc_data.data(), rr_rc_data.size())
	, rr_switch_inf_(rr_switch_inf.data(), rr_switch_inf.size())
	, rr_node_route_inf_(rr_node_route_inf.data(), rr_node_route_inf.size())
	, router_stats_(nullptr)
	, router_debug_(false) {
	heap_.init_heap(grid);
	heap_.set_prune_limit(rr_nodes_.size(), kHeapPruneFactor * rr_nodes_.size());
	}

Then you just use the cached copy rr_graph_ in these functions.

[ethanroj23]

@tangxifan I am trying to add the RRGraphView to the cache with

const RRGraphView& rr_graph, and

, rr_graph_(rr_graph)

But when I try this, the compiler gives me the following warning

error: use of deleted function ‘RRGraphView::RRGraphView(const RRGraphView&)’

Do you know what I am missing? I know that the copy constructors have been disabled in RRGraphView, but perhaps I am supposed to copy to cache in a different way.

[tangxifan]

Yes. You can consider to use the pointer

vtr-verilog-to-routing/vpr/src/route/rr_graph_uxsdcxx_serializer.h

Line 1853 in 2e0bafe

RRGraphView* rr_graph_;

which works fine in the reader/writer class.

[tangxifan]

Anyhow, I think the use of const RRGraphView& is better. But not sure the compiler spits out such error.
@hzeller If you have time, can you look into this and share your opinion? Thanks!

[vaughnbetz]

This code snippet looks like a call to the deleted copy constructor (but I am just going by the conversation, so I don't see the code in context):
, rr_graph_(rr_graph)

[ethanroj23]

@vaughnbetz, I think you are correct. I have implemented the RRGraphView* as @tangxifan suggested, but we could also modify RRGraphView.h so that the copy constructor was no longer deleted if that is what we wanted to do.

[vaughnbetz]

No it will be expensive to copy so a pointer is better. Vaughn

…

On Aug 10, 2021, at 2:00 PM, ethanroj23 ***@***.***> wrote:  @ethanroj23 commented on this pull request. In vpr/src/route/connection_router.cpp: > //To node info - auto rc_index = rr_nodes_.node_rc_index(RRNodeId(to_node)); + auto rc_index = rr_graph.node_rc_index(RRNodeId(to_node)); @vaughnbetz, I think you are correct. I have implemented the RRGraphView* as @tangxifan suggested, but we could also modify RRGraphView.h so that the copy constructor was no longer deleted if that is what we wanted to do. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[ethanroj23]

No it will be expensive to copy so a pointer is better. Vaughn
…
On Aug 10, 2021, at 2:00 PM, ethanroj23 @.**> wrote:  @ethanroj23 commented on this pull request. In vpr/src/route/connection_router.cpp: > //To node info - auto rc_index = rr_nodes_.node_rc_index(RRNodeId(to_node)); + auto rc_index = rr_graph.node_rc_index(RRNodeId(to_node)); @vaughnbetz, I think you are correct. I have implemented the RRGraphView as @tangxifan suggested, but we could also modify RRGraphView.h so that the copy constructor was no longer deleted if that is what we wanted to do. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

Ok, sounds good. I will keep it as I have it for now.

[tangxifan]

@ethanroj23 Look good to me. Thanks for the effort.

[vaughnbetz]

Looks mostly good, but I think you can pass const RRGraphView* pointers in several places, and then use const contexts (g_vtr_context.device().rr_graph).

[vaughnbetz]

Changes look good; thanks. Will merge once CI goes green.

[ethanroj23]

@nelsobe here is the PR

[ethanroj23]

@vaughnbetz I believe you were out last week, but could you take a look / merge this when you have a chance? Thanks

[tangxifan]

Link to #1868 So that we keep tracking.