Skip to content

Add a new API set_node_ptc_num() to RRGraphBuilder #1865

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Add a new API set_node_ptc_num() to RRGraphBuilder #1865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8e9df79
Adding api set_node_ptc_num to rr_graph_builder.h
Sep 30, 2021
d43fd06
Replacing api set_node_ptc_num/set_ptc_num from related .cpp/.h files
Sep 30, 2021
3184a02
Removing old api set_ptc_num from rr_node.cpp/.h
Sep 30, 2021
7db718a
Fixing typo
Sep 30, 2021
411c891
Adding comments about api set_node_ptc_num
Sep 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions vpr/src/device/rr_graph_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,13 @@ class RRGraphBuilder {
node_storage_.set_node_coordinates(id, x1, y1, x2, y2);
}

/** @brief Set the node_ptc_num; The ptc (pin, track, or class) number is an integer
* that allows you to differentiate between wires, pins or sources/sinks with overlapping x,y coordinates or extent.
* This is useful for drawing rr-graphs nicely.*/
Copy link
Contributor

Choose a reason for hiding this comment

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

The set_node_ptc_num is a tricky API because the set_ptc_num carries different meanings for different type of nodes.
You can see that there are other APIs which try to clarify this, for example,

  • set_node_track_num() is for CHANX and CHANY nodes
  • set_node_pin_num() is for OPIN and IPIN nodes
  • set_node_class_num() is for SOURCE and SINK nodes

Therefore, I am thinking about if we should

  • refactor all the APIs above in one pull request
  • evaluate if we should deprecate the API set_node_ptc_num() and use other APIs instead. If we cannot, I am o.k. to keep the API.

@vaughnbetz Please share your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hariszafar-lm I have talked to @vaughnbetz today. He agrees on the roadmap here. Please give a try. Also remember to fix the merging conflicts.

Copy link
Author

Choose a reason for hiding this comment

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

@tangxifan Sure, I'll start it from today. thanks

inline void set_node_ptc_num(RRNodeId id, short new_ptc_num) {
node_storage_.set_node_ptc_num(id, new_ptc_num);
}

/* -- Internal data storage -- */
private:
/* TODO: When the refactoring effort finishes,
Expand Down
2 changes: 1 addition & 1 deletion vpr/src/route/clock_connection_builders.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ RRNodeId RoutingToClockConnection::create_virtual_clock_network_sink_node(int x,
}
int ptc = max_ptc + 1;

rr_graph.set_node_ptc_num(node_index, ptc);
rr_graph_builder.set_node_ptc_num(node_index, ptc);
Copy link
Contributor

Choose a reason for hiding this comment

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

This node is a SINK, so we should be able to use the set_node_class_num() api here now. Best to avoid the set_node_ptc_num () (which is more vague) as much as possible.

rr_graph_builder.set_node_coordinates(node_index, x, y, x, y);
rr_graph_builder.set_node_capacity(node_index, 1);
rr_graph.set_node_cost_index(node_index, RRIndexedDataId(SINK_COST_INDEX));
Expand Down
6 changes: 3 additions & 3 deletions vpr/src/route/rr_graph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1433,7 +1433,7 @@ static void build_rr_sinks_sources(RRGraphBuilder& rr_graph_builder,
float R = 0.;
float C = 0.;
L_rr_node.set_node_rc_index(inode, find_create_rr_rc_data(R, C));
L_rr_node.set_node_ptc_num(inode, iclass);
rr_graph_builder.set_node_ptc_num(inode, iclass);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use set_node_class_num instead (this is for SINKs and SOURCEs).

}

/* Connect IPINS to SINKS and initialize OPINS */
Expand Down Expand Up @@ -1481,7 +1481,7 @@ static void build_rr_sinks_sources(RRGraphBuilder& rr_graph_builder,
float R = 0.;
float C = 0.;
L_rr_node.set_node_rc_index(inode, find_create_rr_rc_data(R, C));
L_rr_node.set_node_ptc_num(inode, ipin);
rr_graph_builder.set_node_ptc_num(inode, ipin);
Copy link
Contributor

Choose a reason for hiding this comment

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

Use set_node_pin_num() instead


//Note that we store the grid tile location and side where the pin is located,
//which greatly simplifies the drawing code
Expand Down Expand Up @@ -1671,7 +1671,7 @@ static void build_rr_chan(RRGraphBuilder& rr_graph_builder,
float C = length * seg_details[track].Cmetal();
L_rr_node.set_node_rc_index(node, find_create_rr_rc_data(R, C));

L_rr_node.set_node_ptc_num(node, track);
rr_graph_builder.set_node_ptc_num(node, track);
Copy link
Contributor

Choose a reason for hiding this comment

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

use set_node_track_num() instead.

Copy link
Author

@m-hariszafar m-hariszafar Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hello @tangxifan and @vaughnbetz,
I replaced this one with set_node_track_num() and it is firing an exception of Attempted to set RR node 'track_num' for non-CHANX/CHANY type '(null)'. Do we have an alternative or we should keep with the previous one set_node_ptc_num()?
As it is passing with set_node_ptc_num().

Copy link
Author

Choose a reason for hiding this comment

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

It is failing for all basic to strong tests.
test-runs

Copy link
Contributor

Choose a reason for hiding this comment

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

@hariszafar-lm This is because the set_node_type() is called after this API. If you move the LINE 1675 before this line. it should work.

L_rr_node.set_node_type(node, chan_type);
L_rr_node.set_node_direction(node, seg_details[track].direction());
}
Expand Down
2 changes: 1 addition & 1 deletion vpr/src/route/rr_graph_uxsdcxx_serializer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ class RrGraphSerializer final : public uxsd::RrGraphBase<RrGraphContextTypes> {
RRNodeId node_id = node.id();

rr_graph_builder_->set_node_coordinates(node_id, xlow, ylow, xhigh, yhigh);
node.set_ptc_num(ptc);
rr_graph_builder_->set_node_ptc_num(node_id, ptc);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only call that really needs to set a ptc_num, no matter what type of node it is. If the node type is already set, we could replace this with an if statement that checks the type and calls the right set_node_xx_num() call, and then delete the set_node_ptc_num() method.

Copy link
Contributor

Choose a reason for hiding this comment

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

If that isn't easily doable, we can leave this call and the method.

Copy link
Author

Choose a reason for hiding this comment

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

@tangxifan what do you suggest on this? Either we should keep it to the set_node_ptc_num() method or else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. We should keep the set_node_ptc_num() API here

Copy link
Author

Choose a reason for hiding this comment

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

Okay, thanks for quick response

return inode;
}
inline void finish_node_loc(int& /*inode*/) final {}
Expand Down
4 changes: 0 additions & 4 deletions vpr/src/route/rr_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,6 @@ void t_rr_node::set_type(t_rr_type new_type) {
storage_->set_node_type(id_, new_type);
}

void t_rr_node::set_ptc_num(short new_ptc_num) {
storage_->set_node_ptc_num(id_, new_ptc_num);
}

void t_rr_node::set_pin_num(short new_pin_num) {
storage_->set_node_pin_num(id_, new_pin_num);
}
Expand Down
1 change: 0 additions & 1 deletion vpr/src/route/rr_node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,6 @@ class t_rr_node {
public: //Mutators
void set_type(t_rr_type new_type);

void set_ptc_num(short);
void set_pin_num(short); //Same as set_ptc_num() by checks type() is consistent
void set_track_num(short); //Same as set_ptc_num() by checks type() is consistent
void set_class_num(short); //Same as set_ptc_num() by checks type() is consistent
Expand Down