-
Notifications
You must be signed in to change notification settings - Fork 430
Now direct connection builder checks the port name of destination subtile #3320
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
…ify source subtile to single destination subtile
|
@vaughnbetz This PR is ready for your review. |
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.
A couple of minor suggestions in the comments.
| clb_to_clb_directs[i].to_clb_type = physical_tile; | ||
|
|
||
| tile_port = find_tile_port_by_name(physical_tile, port_name); | ||
| // Cache the destination port name as the pin index is not enough to identify if the destination subtile is the one we want!!! |
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 nit: we don't need the !!! at the end.
If there's something you want to emphasize about this test I think you'll have to explain it. But just adding !!! in comments will be a distraction.
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.
Understood. The exclamation has been removed.
| } | ||
| } | ||
| VTR_ASSERT(to_sub_tile != nullptr); | ||
| /* Check if the to port is the one from the subtile, if not, pass as this is not the destination */ |
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 nit: we've moved to
// Use double slash for implementation comments
so if you can change to that style it would be good.
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.
Sure. I will keep this convention in future code changes.
|
@vaughnbetz I have addressed the code comments. I merge this PR as CI is green. If you see any issue, I can address them in a follow-up PR. |
Description
This is a follow-up PR on PR #3319
It adds the dedicated testcase to validate
The testcase will trigger another bug:
The
clb_to_clb_directsrelies on pin index to find the destination pin of subtiles. This is not reliable, which may cause wrong direct connections to be added.Take the example of the testcases in this PR,
The line expects to build direct connections from
MULT[1]toRAM[0]. However, when checking if there are any connections required fromMULT[2]toMULT[1], the current builder will mistakenly add an edge.This is due to that the pin index of
RAM[0]is always within the range ofMULT[1]andMULT[2](RAM has more pins than MULT).This PR adds the port name to be checked when building the direct connection, to avoid such bug.
Related Issue
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist: