RRGraphView::node_capacity() Implementation

[ethanroj23]

Description

In this PR, I have implemented RRGraphView::node_capacity() throughout VTR. Every time device_ctx.rr_nodes[index].capacity() was called has been replaced with rr_graph.node_capacity(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 in the table below.

	VTR Before These Changes	With Changes in this PR
vtr_flow_elapsed_time	1	0.989022099
odin_synth_time	1	1.041313002
abc_synth_time	1	1.001016736
max_vpr_mem	1	0.999841509
pack_time	1	0.998665715
place_time	1	1.00707815
min_chan_width_route_time	1	0.968194345
crit_path_route_time	1	0.969866289

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

[ethanroj23]

@vaughnbetz, @hzeller, @tangxifan, here is my next PR for the RRGraphView API. This one implements node_capacity(). If you could review it when you have some time, that would be great! Thanks!

[vaughnbetz]

How much is this function used? We could deprecate it and replace the call sites, since it just gets an id from the node and then calls another function.

[ethanroj23]

I have now removed it. I added include "globals.h" to the top of some of the files in order to access the rr_graph, so hopefully that won't cause any issues. I am going to run qor testing again now.

[vaughnbetz]

QoR and the changes both look good. I just have one comment/question above.
Also, any idea why only a subset of tests have been run on this PR? vtr_reg_night* have not launched. Is there a dependency where certain basic tests must pass first before those launch? And are those basic tests (e.g. odinII) taking an unusually long time? Or is something else going on (e.g. kokoro tests not launching)?

[tangxifan]

@ethanroj23 This pull request looks clean to me. The number of modified lines matches well with my results on 'grep capacity'. Just some minor changes required, and I think it is ready to merge.

[tangxifan]

Actually, introducing global variable is a critical concern to me. It makes the code hard to maintain because it is hard to know what are the required data structure for each function.

This motivates me to review what the rr_graph_uxsdcxx.h is doing. It actually creates its own context in this file, being independent from the RRGraph and other VPR data structure. This looks clean to me. It makes sense that it should have it own node_capacity() function so that they are isolated from RRGraph data structure.

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

Lines 803 to 805 in 6e442d1

	inline unsigned int get_node_capacity(const t_rr_node& node) final {
	return node.capacity();
	}

So, I suggest not to introduce the global variables to this file. But we should update the API in the rr_graph_uxsdcxx_serializer.h. Actually, I found how they access the rr_graph inside. We can update the get_node_capacity() like this:

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

Lines 809 to 812 in 6e442d1

	inline uxsd::enum_node_type get_node_type(const t_rr_node& node) final {
	const auto& rr_graph = (*rr_graph_);
	return to_uxsd_node_type(rr_graph.node_type(node.id()));
	}

[tangxifan]

@vaughnbetz I found something in the RRGraph serializer which may conflicts with your previous comments. See if my suggestion makes sense or not.

[vaughnbetz]

OK, I agree with you @tangxifan . I missed that they weren't accessing the same rr_graph data structure, but were instead accessing their own local version. In that case, I agree my earlier comments are wrong and we should revert to what Ethan had (which I think is basically the same code as what you propose above).

Sorry for the churn @ethanroj23

[ethanroj23]

No worries! I have now replaced the code, with the commented out sections of code still removed, so we should be good to go! @vaughnbetz let me know if there is anything else that should be altered here. Thanks!

[tangxifan]

@vaughnbetz Thanks for double-check!

@ethanroj23 I have no concerns on the PR. Look good to me.