RRGraphView rr_segments() Implementation

[umariqbal-rs]

Description

In this PR, I am implementing RRGraphView::rr_segments(PRSegmentId Seg_Id ) throughout VTR. Every time device_ctx.rr_segment[Seg_Id] was called has been replaced with rr_graph.rr_segment(Seg_Id). In order to do this, I followed a pattern similar to that in the last PR.

Related Issue

Motivation and Context

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

How Has This Been Tested?

• All vtr basic and strong tests are passing.

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

[umariqbal-rs]

@tangxifan
I have added RRSegmentId

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

Line 268 in c828072

const vtr::vector<RRSegmentId, t_segment_inf>& segment_inf,

But I got following errors.

I think we also need to add it in line line 1874

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

Line 1874 in c828072

const std::vector<t_segment_inf>& segment_inf_;

but when find the call hierarchy of segment_inf_; its called in line 1152,1184,1553 and others.

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

Line 1152 in c828072

inline int get_segment_id(const t_segment_inf*& segment) final {

which causes me to change in many files, and pass RRSegmentId there.
Can you please review how we will move next?Because I already made changes in a lot of files that didn't worked.

[tangxifan]

@umariqbal-rs If the changes are within the file rr_graph_uxsdcxx_serializer.h, it is o.k.

You may need to change some APIs, where convert the int id to RRSegmentId.
Also

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

Line 1182 in bae983f

inline const t_segment_inf* add_segments_segment(void*& /*ctx*/, int id) final {

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

Line 1154 in bae983f

inline int get_segment_id(const t_segment_inf*& segment) final {

For the alloc_and_load_rr_indexed_data(), you may just need to do a data type conversion here (from std::vector to vtr::vector), in order to avoid large code changes

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

Lines 1554 to 1557 in bae983f

	alloc_and_load_rr_indexed_data(
	segment_inf_,
	*wire_to_rr_ipin_switch_,
	base_cost_type_);

[umariqbal-rs]

@tangxifan
I have added RRSegmentId in

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

Line 1152 in fc5e3d6

inline int get_segment_id(const t_segment_inf*& segment) final {

and

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

Line 1181 in fc5e3d6

return &segment_inf_.at(RRSegmentId(id));

and where it is needed in rr_graph_uxsdcxx_serializer.h
I also done type conversion from std to vtr in

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

Lines 1554 to 1557 in bae983f

	alloc_and_load_rr_indexed_data(
	segment_inf_,
	*wire_to_rr_ipin_switch_,
	base_cost_type_);

this function is declared in rr_graph_indexed_data.cpp

vtr-verilog-to-routing/vpr/src/route/rr_graph_indexed_data.cpp

Line 56 in fc5e3d6

void alloc_and_load_rr_indexed_data(const vtr::vector<RRSegmentId, t_segment_inf>& segment_inf,

when we followup its call hierarchy it needs us to change in many others files and functions.
Although I have changed in where it is needed.
But I want your suggestions how can I avoid these large code changes and avoiding to stuck in last.
Please review it and suggest me how can i move forward.

[tangxifan]

@umariqbal-rs

For the alloc_and_load_rr_indexed_data(), you just do a data type conversion before calling this function (from std::vector to vtr::vector). I suggested not to change the data type of the arguments of the function.
Just like what you did in the vpr/src/route/rr_graph.cpp file.

[umariqbal-rs]

@tangxifan
I have done what you said above from std to vtr

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

Line 1554 in d5076db

size_t num_segments = segment_inf_.size();

Is this you suggested me to do?

But still I am having an error

In function alloc_and_load_rr_indexed_data() at segment_inf,
‘std::vector<t_segment_inf>’ is an inaccessible base of ‘vtr::vector<vtr::StrongId<rr_segment_id_tag, short int>, t_segment_inf>’.
this is the error I am having.

Can you Please review it and suggest me how to resolve this ?

[tangxifan]

@umariqbal-rs

You need to create a local copy of the segment_inf here

/* Create a temp copy to convert from vtr::vector to std::vector
 * This is required because the ``alloc_and_load_rr_indexed_data()`` function supports only std::vector data type for ``rr_segments``
 * Note that this is a dirty fix (to avoid massive code changes)
 * TODO: The ``alloc_and_load_rr_indexed_data()`` function should embrace ``vtr::vector`` for ``rr_segments``
 */
std::vector<RRSegmentId, t_segment_inf> temp_rr_segs;
rr_seg.reserve(segment_inf_.size());
for (const auto& temp_rr_seg : temp_rr_segs) {
  rr_seg.push_back(temp_rr_seg);
}
alloc_and_load_rr_indexed_data(
            temp_rr_segs,
            *wire_to_rr_ipin_switch_,
            base_cost_type_);

[tangxifan]

@umariqbal-rs please also resolve merging conflicts.

[umariqbal-rs]

@tangxifan
I have added what you suggested me to do.

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

Line 1555 in 62ed037

std::vector<RRSegmentId, t_segment_inf> temp_rr_segs;

But I encountered with following errors.

no type named ‘value_type’ in ‘struct t_segment_inf’
  rebind<_Tp>::other _Tp_alloc_type;

and

 no members matching ‘std::vector<vtr::StrongId<rr_segment_id_tag, short int>, t_segment_inf>::_Base {aka std::_Vector_base<vtr::StrongId<rr_segment_id_tag, short int>, t_segment_inf>}::_M_get_Tp_allocator’ in ‘std::vector<vtr::StrongId<rr_segment_id_tag, short int>, t_segment_inf>::_Base’ {aka ‘struct std::_Vector_base<vtr::StrongId<rr_segment_id_tag, short int>, t_segment_inf>’}
       using _Base::_M_get_Tp_allocator;

Can you Please review it and suggest me how can we move forward.

[tangxifan]

@umariqbal-rs Sorry for the typo. It should be std::vector<t_segment_inf>

[tangxifan]

@umariqbal-rs I see errors in read_rr_graph. Please double check in the rr_graph_reader.cpp files if you have fill the rr_segments correctly.

[umariqbal-rs]

@tangxifan
I have checked it .
This is you suggested me to replace
this line
//device_ctx.rr_segments = segment_inf;
with this

    size_t num_segments = segment_inf.size();
    device_ctx.rr_segments.reserve(num_segments);
    for (long unsigned int iseg = 0; iseg < num_segments; ++iseg) {
    	device_ctx.rr_segments.push_back(segment_inf[iseg]);
    }

vtr-verilog-to-routing/vpr/src/route/rr_graph_reader.cpp

Line 49 in 62ed037

size_t num_segments = segment_inf.size();

can you Please review it is anything i did wrong?

[tangxifan]

@umariqbal-rs I think it is because we did not clear the device_ctx.rr_segments before reserve.

Can you add it to this file

device_ctx.rr_segments.clear();

vtr-verilog-to-routing/vpr/src/route/rr_graph.cpp

Line 1352 in 0994698

device_ctx.rr_switch_inf.clear();

[umariqbal-rs]

tangxifan
I have changed avove what you mentioned to do but still issues in Unit Tests and R Strong test.
I think issue is in rr_graph_reader.cpp.
Here are changes I did in rr_graph_reader.cpp.
vtr::vector<RRSegmentId, t_segment_inf> temp_segment_inf;
I created a temporary vtr::vector copy with RRSegmentId, because we need to assign it to device_ctx.rr_segments = temp_segment_inf;
https://github.com/RapidSilicon/vtr-verilog-to-routing/actions/runs/1471678377

Can you Please review and suggest me how can we move forward.

[umariqbal-rs]

QoR tests are complete and quick summary is given below. QoR comparison files are also attached.
vtr_reg_qor_chain_depop test
Flow run time -1.48% on average.
critical path delay 0% on average.
Peak memory usage -0.02% on average
vtr_reg_qor_chain_depop_comp.xlsx

[tangxifan]

@umariqbal-rs Minor changes required. It looks o.k.

[tangxifan]

Looks o.k. to me. Some minor changes required but we can do it in a follow-up PR.

[umariqbal-rs]

QoR tests are complete and quick summary is given below. QoR comparison files are also attached.
vtr_reg_qor_chain_depop test
Flow run time 1% on average.
critical path delay 0% on average.
Peak memory usage 0.42% on average
titan_quick_qor test:
flow run time 0.76% on average.
critical path delay 0% on average.
Peak memory usage 0.01% on average
titan_quick_qor.xlsx
vtr_reg_qor_chain_depop_comp.xlsx

[tangxifan]

@umariqbal-rs Look good. I propose to merge now. Remember to create a follow-up PR to correct the comments.
@vaughnbetz If you see any issue, just flag it. We will follow up.

[tangxifan]

From @vaughnbetz: in future, we should think about more high-level API, e.g.,
seg_timing()
seg_delay()
seg_length()

[tangxifan]

@umariqbal-rs I have discussed with @vaughnbetz today this PR. We have the green light to merge.
However, we have a follow-up PR to create, which should do the following task

• Fix the bug on the code comments (as I pointed out in code review)
• Remove the rr_segments from DeviceContext. Since we have updated all the client functions, we can safely remove it from DeviceContext. It will avoid confusing developers and keep the idea clear: RRGraphView should be the only data structure to store any routing-resource -related data.

Let me know what you think.

[umariqbal-rs]

OK sure. I am currently working on edge_switch() .we will follow rr_segments along with edge_swtich and edge_sink_node().
Is that fine ?

[tangxifan]

@umariqbal-rs I do not suggest to do so. It may increase your debugging complexity. But I am open. If you do not see any challenge, we can go in this way.