Refactor TA files and format and document TA support files.#9677
Refactor TA files and format and document TA support files.#9677alokkumardalei-wq wants to merge 4 commits into
Conversation
alokkumardalei-wq
force-pushed
the
drt_refactor_ta
branch
from
0e7b723 to
36a4386
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring of the track assignment (TA) logic. It successfully breaks down large, complex functions into smaller, more manageable helpers, which greatly improves code readability and maintainability. The changes also enforce a consistent coding style, such as using snake_case for variable names.
I've found a critical issue where the program terminates with exit(1) on an error, which should be replaced with proper exception handling. Additionally, there are a couple of medium-severity issues related to inconsistent logging (including the use of magic numbers for message IDs) and container usage that should be addressed to further improve code quality.
Overall, this is a valuable refactoring effort.
| if (best_track_idx == -1) { | ||
| auto* guide = iroute->getGuide(); | ||
| const odb::Rect box = guide->getBBox(); | ||
| std::cout << "Error: assignIroute_bestTrack select no track for " | ||
| << guide->getNet()->getName() << " @(" << box.xMin() / dbu << ", " | ||
| << box.yMin() / dbu << ") (" << box.xMax() / dbu << ", " | ||
| << box.yMax() / dbu << " " | ||
| << getDesign()->getTech()->getLayer(lNum)->getName() | ||
| << " idx1/2=" << idx1 << "/" << idx2 << '\n'; | ||
| << getDesign()->getTech()->getLayer(layer_num)->getName() | ||
| << " idx_1/2=" << idx_1 << "/" << idx_2 << '\n'; | ||
| exit(1); | ||
| } |
There was a problem hiding this comment.
Calling exit(1) is not a good practice for error handling within a library function as it abruptly terminates the entire application. It's better to log the error using logger_->error and then throw an exception (e.g., std::runtime_error or a custom exception like utl::DRTException) to allow the caller to handle the error gracefully. Additionally, the 0 used as a message ID in the suggested logger_->error call is a magic number and should be replaced with a named constant for better maintainability and clarity, as per the guideline on tunable parameters.
if (best_track_idx == -1) {
auto* guide = iroute->getGuide();
const odb::Rect box = guide->getBBox();
logger_->error(DRT,
0, // A suitable message ID should be used.
"assignIroute_bestTrack could not select a track for net {} "
"in box ({}, {}) ({}, {}) on layer {} with track indices "
"({}, {}).",
guide->getNet()->getName(),
box.xMin() / dbu,
box.yMin() / dbu,
box.xMax() / dbu,
box.yMax() / dbu,
getDesign()->getTech()->getLayer(layer_num)->getName(),
idx_1,
idx_2);
throw std::runtime_error("Track assignment failed.");
}References
- Define tunable parameters as named constants instead of using hardcoded magic numbers.
| } | ||
| workers.back().push_back(std::move(uworker)); | ||
| worker_batches.back().push_back(std::move(worker_uptr)); |
There was a problem hiding this comment.
For consistency with the horizontal case (which uses emplace_back), it would be better to use emplace_back here as well.
| worker_batches.back().push_back(std::move(worker_uptr)); | |
| worker_batches.back().emplace_back(std::move(worker_uptr)); |
| std::cout | ||
| << "Warning: via layer connected to non-routing layer, skipped in " | ||
| "modMinSpacingCostVia\n"; |
There was a problem hiding this comment.
Warnings should be logged using the logger_ object for consistency, rather than printing directly to std::cout. This allows for better control over log verbosity and output streams. Please use a suitable message ID instead of 0. This 0 is a magic number and should be replaced with a named constant for better maintainability and clarity, as per the guideline on tunable parameters.
logger_->warn(DRT, 0, "Via layer connected to non-routing layer, skipped in modMinSpacingCostVia.");References
- Define tunable parameters as named constants instead of using hardcoded magic numbers.
| std::cout | ||
| << "Warning: via layer connected to non-routing layer, skipped in " | ||
| "modMinSpacingCostVia\n"; | ||
| "modCutSpacingCost\n"; |
There was a problem hiding this comment.
Warnings should be logged using the logger_ object for consistency, rather than printing directly to std::cout. This allows for better control over log verbosity and output streams. Please use a suitable message ID instead of 0. This 0 is a magic number and should be replaced with a named constant for better maintainability and clarity, as per the guideline on tunable parameters.
logger_->warn(DRT, 0, "Via layer connected to non-routing layer, skipped in modCutSpacingCost.");References
- Define tunable parameters as named constants instead of using hardcoded magic numbers.
alokkumardalei-wq
force-pushed
the
drt_refactor_ta
branch
from
36a4386 to
cc64bfd
Compare
|
@osamahammad21 please suggest additional validation (eg ispd) |
alokkumardalei-wq
force-pushed
the
drt_refactor_ta
branch
from
cc64bfd to
5a79e5c
Compare
Refactor track-assignment internals to reduce complexity while keeping behavior unchanged. - split TA init/assign logic into focused helpers - simplify cut-spacing and best-track selection control flow - clean up TA support files for readability and consistent Google-style C++ formatting Closes The-OpenROAD-Project#5614. Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
alokkumardalei-wq
force-pushed
the
drt_refactor_ta
branch
from
5a79e5c to
e4b58db
Compare
alokkumardalei-wq
changed the title
alokkumardalei-wq
changed the title
alokkumardalei-wq
changed the title
|
Thanks @maliberty for pointing this out. It's right that just the bazel build test would not suffice to validate the working, we need more tests like for example ispd tests and other tests. But you can see ispd tests are failing as because test framework does a strict text diff against golden DEF files and the generated DEF has two extra ROUTED Metal2. I you want I can try to fix this by manually updating MANUAL_FOR_BAZEL_TESTS in BUILD with adding these cases. |
|
In general refactoring is not expected to change the results. Please investigate why there are differences in the DEF |
|
Hello @maliberty do you mean the After and Before outputs are ok of ispd test and I don't need to change anything and provide concrete evidence why there differences in DEF or anything else?? |
|
Refactoring to me means to reorganize code without functional change. If the results change then it isn't a refactoring or you have a bug in your refactoring. |
|
Hello @maliberty You can clearly see here now also our May be I am foolish but I guess there is no error in my PR for the issue and it the issue from upstream itself ,if would like to allow me to update the .defok file I will definitely do that. Please let me know If I am correct about and proceed with further process. Again thank you so much for your time and would be happy receive your feedback further. |
|
Also when you have time you can review these Open PRs: |
|
You can see it passing in the nightly CI at https://jenkins.openroad.tools/blue/organizations/jenkins/OpenROAD-Nightly-Public/detail/OpenROAD-Nightly-Public/2716/pipeline |
|
@maliberty Thank you for checking the CI! I am developing locally on a macOS ARM64 machine. I ran a definitive test to prove this:
Then I researched and found that OpenRod But on my Linux system CI, both tracks likely tie in, Because our refactored code produces the exact Thank you so much for your time and looking forward for further feedback. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| frLayerNum& layer_num, | ||
| int& idx1, | ||
| int& idx2); | ||
| int assignIroute_bestTrack(taPin* iroute, |
There was a problem hiding this comment.
warning: function 'drt::FlexTAWorker::assignIroute_bestTrack' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]
int assignIroute_bestTrack(taPin* iroute,
^Additional context
src/drt/src/ta/FlexTA_assign.cpp:1105: the definition seen here
int FlexTAWorker::assignIroute_bestTrack(taPin* iroute,
^src/drt/src/ta/FlexTA.h:403: differing parameters are named here: ('idx1', 'idx2'), in definition: ('idx_1', 'idx_2')
int assignIroute_bestTrack(taPin* iroute,
^| } | ||
| const auto& track_locs = getTrackLocs(layer_num); | ||
| int start_track_idx = static_cast<int>( | ||
| std::lower_bound(track_locs.begin(), track_locs.end(), pin_coord) |
There was a problem hiding this comment.
warning: use a ranges version of this algorithm [modernize-use-ranges]
| std::lower_bound(track_locs.begin(), track_locs.end(), pin_coord) | |
| std::ranges::lower_bound(track_locs,, pin_coord) |
| && getTech()->getLayer(layer_num - 2)->getType() | ||
| == dbTechLayerType::ROUTING) { | ||
| auto cut_layer = getTech()->getLayer(layer_num - 1); | ||
| auto via = std::make_unique<frVia>(cut_layer->getDefaultViaDef()); |
There was a problem hiding this comment.
warning: no header providing "drt::frVia" is directly included [misc-include-cleaner]
src/drt/src/ta/FlexTA_init.cpp:20:
- #include "db/taObj/taFig.h"
+ #include "db/obj/frVia.h"
+ #include "db/taObj/taFig.h"- Use std::ranges::lower_bound in FlexTA_assign.cpp to fix modernize-use-ranges warning - Fix inconsistent parameter names between definition and declaration for assignIroute algorithms - Add missing frVia.h include in FlexTA_init.cpp Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
alokkumardalei-wq
force-pushed
the
drt_refactor_ta
branch
from
523f107 to
ac92ca0
Compare
Signed-off-by: Alok Kumar Dalei <alokkumardalei2@gmail.com>
| } | ||
| const auto& track_locs = getTrackLocs(layer_num); | ||
| int start_track_idx = static_cast<int>( | ||
| std::ranges::lower_bound(track_locs, pin_coord) - track_locs.begin()); |
There was a problem hiding this comment.
warning: no header providing "std::ranges::lower_bound" is directly included [misc-include-cleaner]
src/drt/src/ta/FlexTA_assign.cpp:3:
- #include <algorithm>
+ #include <bits/ranges_algo.h>
+ #include <algorithm>| trackNum++; | ||
| for (auto& tp : getDesign()->getTopBlock()->getTrackPatterns(layer_num)) { | ||
| if ((getDir() == dbTechLayerDir::HORIZONTAL | ||
| && tp->isHorizontal() == false) |
There was a problem hiding this comment.
warning: redundant boolean literal supplied to boolean operator [readability-simplify-boolean-expr]
| && tp->isHorizontal() == false) | |
| && !tp->isHorizontal()) |
| if ((getDir() == dbTechLayerDir::HORIZONTAL | ||
| && tp->isHorizontal() == false) | ||
| || (getDir() == dbTechLayerDir::VERTICAL | ||
| && tp->isHorizontal() == true)) { |
There was a problem hiding this comment.
warning: redundant boolean literal supplied to boolean operator [readability-simplify-boolean-expr]
| && tp->isHorizontal() == true)) { | |
| && tp->isHorizontal())) { |
|
@alokkumardalei-wq Refactoring PRs tend to become very large and difficult to review if they’re not properly split. For context, the PA refactor was broken down into around 10 separate PRs. To make this reviewable, please divide this into smaller, focused PRs. Each PR should contain only one type of change, for example:
The goal is to keep each PR atomic and easy to review independently. |
Signed-off-by: Alok Kumar Dalei <alokkumardalei2@gmail.com>
|
Thank you @osamahammad21 for the feedback , I will definitely divide this into small PRs and will notify you very soon |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 21 days if no further activity occurs. Remove the |
github-actions
Bot
added
the
Stale
3 participants
What I have resolve with this PR?
Fixes #5614 .
I have:
What got resolved?
FlexTA_init.cppandFlexTA_assign.cpp.FlexTA.cppFlexTA_end.cppFlexTA_graphics.cppFlexTA_rq.cppFlexTA.hupdated only for private helper declarations required by the refactor.Validation
Expected output:
bazel test //src/drt/test:ta_ap_aligned-tcl_test //src/drt/test:ta_pin_aligned-tcl_test --test_output=errorsExpected output:
bazel test //src/drt/test:obstruction-tcl_test //src/drt/test:top_level_term-tcl_test //src/drt/test:via_access_layer-tcl_test --test_output=errorsExpected output:
bazel test //src/drt/test:gc_unittest --test_output=errorsExpected output:
ispd tests:
bazel test //src/drt/test:ispd18_sample-tcl_test --test_output=errorsRun this command.
Expected output before (refactor):
Expected output after (refactor):
We found that in both the cases the output is same and it's failing.
Why??
These ispd tests are failing because the test framework does a strict text diff against golden DEF files and therefore it is showing same output in both the cases.