-
Notifications
You must be signed in to change notification settings - Fork 429
Now direct connection builder in tileable rr_graph supports any subtile index as the from_pin #3318
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.
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
Conversation
…alidate the direct connections across subtiles
… connection for tileable rr_graph
|
@vaughnbetz This PR is ready for your review. I notice this feature is missing for the regular rr_graph builder. We can add it if you want. I have added @amin1377 as a potential reviewer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, except it needs a Doxygen comment on a new utility function.
I also find the (true == swap) style odd, but if you really like it I'm not dogmatic on that.
Adding this to the default rr-graph builder is a good idea, so if you can do that it would be good.
Does any documentation need to be updated?
|
|
||
| bool is_empty_type(t_physical_tile_type_ptr type); | ||
| bool is_empty_type(t_logical_block_type_ptr type); | ||
| std::vector<int> find_sub_tile_indices_by_port_name(t_physical_tile_type_ptr type, std::string_view port_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a doxygen comment describing the function purpose, return type and parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code comments have been added.
| int relative_ipin = UNDEFINED; | ||
| if (clb_to_clb_directs[i].to_clb_pin_start_index | ||
| > clb_to_clb_directs[i].to_clb_pin_end_index) { | ||
| if (true == swap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Odd style. I'd just put
if (swap)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapted
| } | ||
| } else { | ||
| relative_ipin = clb_to_clb_directs[i].to_clb_pin_start_index - offset; | ||
| if (true == swap) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Odd style. I'd use:
if (swap)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adapted
…or regular rr_graph generator
…s between subtiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Xifan. I’d suggest adding the figure you included in the PR description to this document (Link)
| return get_class_num_from_pin_physical_num(physical_type, pin_physical_num); | ||
| } | ||
|
|
||
| std::vector<int> find_sub_tile_indices_by_port_name(t_physical_tile_type_ptr type, std::string_view port_name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tangxifan: I’m a little confused here. Let’s say I have a tile that has 5 subtiles of the same type. All of those subtiles would have the same port name… How would you get the index of a subtile then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will get all the indices of the subtiles as long as the subtile has a port in the required name. These subtiles can all be the starting point of a direct connection.
…et at tile-level (pin location)
…ting/vtr-verilog-to-routing into xt_tileable_direct_strong
|
@vaughnbetz Thank you for the quick feedback. I have addressed the code comments. Now the regular rr_graph generator supports the direct connections across subtiles. New testcases have to added to validate the regular rr_graph generator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nit in a comment, but other than that looks good to go.
vpr/src/util/vpr_utils.h
Outdated
| bool is_empty_type(t_logical_block_type_ptr type); | ||
|
|
||
| /** | ||
| * @brief Returns the indices of sub tiles in a given physical type which contains the ports in a specified name. It will return all the sub tiles that contain the required port. Note that the sub tiles may be in different types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits: I think this should be:
"which contain the specified port name."
"Note that the sub tiles may be of different types"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
@vaughnbetz @amin1377 All the comments are addressed. CI is green. Please suggest if I can merge this PR. |
|
Please go ahead and merge.VaughnOn Oct 24, 2025, at 7:24 PM, tangxifan ***@***.***> wrote:tangxifan left a comment (verilog-to-routing/vtr-verilog-to-routing#3318)
@vaughnbetz @amin1377 All the comments are addressed. CI is green. Please suggest if I can merge this PR.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Description
This PR aims to enhance the direct connection builder of the tileable routing resource graph builder, with the following new feature:
zin a tile as the starting point of a direct connection. Current direct builder requires the first subtile to be the starting point, which is very restrictive.Related Issue
No issue is related. Stated in the description section.
Motivation and Context
Stated in the description section.
How Has This Been Tested?
Types of changes
Checklist: