-
Notifications
You must be signed in to change notification settings - Fork 759
DRT: Add guide-based tiles flow and improve stubborn-tiles iteration control #9198
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
DRT: Add guide-based tiles flow and improve stubborn-tiles iteration control #9198
Conversation
Signed-off-by: osamahammad21 <[email protected]>
Signed-off-by: osamahammad21 <[email protected]>
Signed-off-by: osamahammad21 <[email protected]>
Signed-off-by: osamahammad21 <[email protected]>
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 Review
This pull request introduces a new guide-based tiles flow to handle stubborn violations, which is a great addition for improving routing quality. The logic for when to invoke this new flow and how to control iteration skipping is well-thought-out. I also appreciate the refactoring to use coordinate-space rectangles, which simplifies the code and improves readability. I have one suggestion to improve the clarity and maintainability of the rectangle merging logic in the new guideTilesFlow function.
| for (auto itr1 = workers.begin(); itr1 != workers.end(); ++itr1) { | ||
| for (auto itr2 = itr1 + 1; itr2 != workers.end(); ++itr2) { | ||
| if (itr1->getDir() == itr2->getDir() && itr1->intersects(*itr2)) { | ||
| itr1->merge(*itr2); | ||
| workers.erase(itr2); | ||
| itr2 = itr1; | ||
| } | ||
| } | ||
| } |
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.
The nested loop for merging worker rectangles is functionally correct, but its implementation using itr2 = itr1; to restart the inner loop is a non-standard pattern which can be confusing for future maintenance.
A goto statement, while often discouraged, can make the intent of restarting the loop scan much clearer in this specific case. This change improves readability without altering the logic. The inefficiency of std::vector::erase in a loop remains, but this is a separate concern from code clarity.
for (auto itr1 = workers.begin(); itr1 != workers.end(); ++itr1) {
restart_inner_loop:
for (auto itr2 = itr1 + 1; itr2 != workers.end(); ++itr2) {
if (itr1->getDir() == itr2->getDir() && itr1->intersects(*itr2)) {
itr1->merge(*itr2);
workers.erase(itr2);
goto restart_inner_loop;
}
}
}|
clang-tidy review says "All clean, LGTM! 👍" |
Nice! 🎉 |
| if (itr1->getDir() == itr2->getDir() && itr1->intersects(*itr2)) { | ||
| itr1->merge(*itr2); | ||
| workers.erase(itr2); | ||
| itr2 = itr1; |
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.
should this be itr1 or itr1+1 as in the loop initialization?
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 should be itr1. Once the loop finishes an iteration and goes back to the for statement, the increment step will advance it to itr1+1.
|
I'm a bit unclear if you could get adjacent workers that would depend on each other. |
You could, and probably would. It's handled in |
The changes include:
This change handle all stubborn violations on gf12 designs.