Skip to content

Conversation

soheilshahrouz
Copy link
Contributor

@soheilshahrouz soheilshahrouz commented Sep 25, 2025

Moves all functions that create nodes and edges for intra-cluster routing resources to separate files.

@github-actions github-actions bot added VPR VPR FPGA Placement & Routing Tool libarchfpga Library for handling FPGA Architecture descriptions lang-cpp C/C++ code labels Sep 25, 2025
@soheilshahrouz soheilshahrouz changed the title [WIP] Refactor: Move intra-cluster RR graph code into dedicated files Refactor: Move intra-cluster RR graph code into dedicated files Sep 25, 2025
Copy link
Contributor

@AlexandreSinger AlexandreSinger left a comment

Choose a reason for hiding this comment

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

LGTM.

I really like this change.

Copy link
Contributor

@amin1377 amin1377 left a comment

Choose a reason for hiding this comment

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

Thanks, Soheil!

/* Connection not at end of the CHANX segment. */
x1 = draw_coords->tile_x[chany_x] + draw_coords->get_tile_width();
if (rr_graph.node_direction(chanx_node) != Direction::BIDIR && (SwitchType)switch_type != SwitchType::SHORT) {
if (rr_graph.node_direction(chanx_node) != Direction::BIDIR && (e_switch_type)switch_type != e_switch_type::SHORT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it’s an easy change, I’d suggest updating the function’s parameter type from short to e_switch_type to avoid casting here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few lines below, switch_type is passed to draw_rr_switch() as an RRSwitchId. I think switch_type is actually a switch id that should be used to retreive a t_rr_switch_inf. Could you please read the code and see if my understanding is correct? If so, casting it to e_switch_type is a mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

This code does look funny. Do we have a special switch index that means short? We seem to be using the same variable as a switch index and a switch type ... is this a bug? It might not cause fatal problems, as it just stops some drawing code, but it seems suspicious.

@amin1377 amin1377 merged commit 6274fd1 into master Sep 29, 2025
30 checks passed
@amin1377 amin1377 deleted the temp_org_rr_g branch September 29, 2025 15:22
Copy link
Contributor

@vaughnbetz vaughnbetz left a comment

Choose a reason for hiding this comment

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

Missed the merge, but here are some suggestions for commenting updates that could be done in a follow-up PR.

/* Connection not at end of the CHANX segment. */
x1 = draw_coords->tile_x[chany_x] + draw_coords->get_tile_width();
if (rr_graph.node_direction(chanx_node) != Direction::BIDIR && (SwitchType)switch_type != SwitchType::SHORT) {
if (rr_graph.node_direction(chanx_node) != Direction::BIDIR && (e_switch_type)switch_type != e_switch_type::SHORT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code does look funny. Do we have a special switch index that means short? We seem to be using the same variable as a switch index and a switch type ... is this a bug? It might not cause fatal problems, as it just stops some drawing code, but it seems suspicious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lang-cpp C/C++ code libarchfpga Library for handling FPGA Architecture descriptions VPR VPR FPGA Placement & Routing Tool

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants