Conversation
![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 introduces a new slew violation fixing algorithm, UvDRC. The implementation is comprehensive, involving a new set of classes for RC tree representation and buffer insertion logic. The changes are well-integrated into the existing Resizer flow with a new uvdrc flag. The code is generally well-structured. My review focuses on code cleanup, clarity, and potential issues in the new implementation and test files. I've identified several areas with commented-out code, TODOs, and misleading variable names that should be addressed. Additionally, a test flow file seems to have a temporary exit command that should be removed.
| utl::metric "RSZ::max_fanout_slack" [expr [sta::max_fanout_check_slack_limit] * 100] | ||
| utl::metric "RSZ::max_capacitance_slack" [expr [sta::max_capacitance_check_slack_limit] * 100] | ||
|
|
||
| exit |
There was a problem hiding this comment.
An exit command has been added here, which will cause the test flow to terminate prematurely. This was likely added for debugging and should be removed to ensure the full test flow can run.
| // root->DebugPrint(resizer_->logger()); | ||
| } | ||
|
|
||
| // TODO: IMPL |
There was a problem hiding this comment.
This TODO comment seems to be stale as the function is implemented. Please remove it.
| // double wire_slew | ||
| // = wire_r * (sol.cap + wire_c) * | ||
| // UvDRCSlewBuffer::K_OPENROAD_SLEW_FACTOR; |
There was a problem hiding this comment.
This commented-out code should be removed to improve readability.
| const sta::Pin* drvr_pin) | ||
| { | ||
| auto network = resizer_->network(); | ||
| // auto sdc_network = network->sdcNetwork(); |
There was a problem hiding this comment.
This line appears to be commented-out debug code. Please remove it.
| // std::unordered_map<odb::Point, RCTreeNodePtr, LocHash, LocEqual> | ||
| // loc_node_map{}; |
There was a problem hiding this comment.
This commented-out code should be removed.
| RCTreeNodePtr root = nullptr; | ||
| { | ||
| auto [locs, loc_map] = InitNetConnections(drvr_pin); | ||
| auto steiner_tree = MakeSteinerTree(drvr_pin, locs, loc_map); |
There was a problem hiding this comment.
To be consistent with the suggested change for the misleading variable name loc_map, please update its usage here as well.
| auto steiner_tree = MakeSteinerTree(drvr_pin, locs, loc_map); | |
| auto steiner_tree = MakeSteinerTree(drvr_pin, locs, pins); |
| // steiner_tree.printTree(resizer_->logger()); | ||
|
|
||
| root = BuildRCTree(drvr_pin, corner, steiner_tree, locs, loc_map); | ||
|
|
||
| PrepareBufferSlots(root, corner); | ||
|
|
||
| // root->DebugPrint(resizer_->logger()); |
There was a problem hiding this comment.
These commented-out lines appear to be for debugging. Please remove them before merging.
| // resizer_->logger()->report( | ||
| // "Best buffer solution for the root node: area = {}", | ||
| // best_solution.buffer_locs.size()); | ||
| // for (auto& p : best_solution.buffer_locs) { | ||
| // resizer_->logger()->report(" Buffer at ({}, {}) with cell {}", | ||
| // p.first->Location().x(), | ||
| // p.first->Location().y(), | ||
| // p.second->name()); | ||
| // } |
There was a problem hiding this comment.
These commented-out lines appear to be for debugging. Please remove them before merging.
| # UvDRC (Univista DRC fixing project) | ||
|
|
||
| ## TODOs | ||
|
|
||
| 1. Support slew & cap margin settings | ||
| 2. Benchmark tests | ||
|
|
||
| ## LIMITATION | ||
|
|
||
| 一些 case 里，OpenROAD 会认为 PD 算法得到的 SteinerTree 有 overlap，判为不合法。此时它会调用 Flute 引擎做一个快速的 virtual routing 得到一颗 RC 树。但我们现有的代码是基于 PD 算法的 SteinerTree 写的，用这颗 RC 树可能 CRASH。 | ||
|
|
||
| ## WEEKLY | ||
|
|
||
| 1. 修 BUG，目前已经能够修复 cap violation | ||
| 2. TODO: 代码进到我们的 branch 里 | ||
| 3. 更多的 BM 测试 | ||
| 4. 研究 OpenROAD 在 Routing 后 fix slew 的代码 |
There was a problem hiding this comment.
This file appears to contain developer notes and is written in Chinese. For consistency with the rest of the project, please consider translating it to English or removing it if it's not intended to be part of the permanent documentation.
| size_t seed = loc.x() * 31 + loc.y(); | ||
| seed = HashCombine(seed, std::hash<int>()(loc.x())); | ||
| seed = HashCombine(seed, std::hash<int>()(loc.y())); | ||
| return seed; |
There was a problem hiding this comment.
The implementation of this hash function can be simplified. The initial seed calculation loc.x() * 31 + loc.y() is redundant since you are already using std::hash and HashCombine. A cleaner approach would be to combine the hashes of x and y coordinates directly.
| size_t seed = loc.x() * 31 + loc.y(); | |
| seed = HashCombine(seed, std::hash<int>()(loc.x())); | |
| seed = HashCombine(seed, std::hash<int>()(loc.y())); | |
| return seed; | |
| size_t seed = 0; | |
| seed = HashCombine(seed, std::hash<int>()(loc.x())); | |
| seed = HashCombine(seed, std::hash<int>()(loc.y())); | |
| return seed; |
|
Commits need to be signed |
|
merge conflicts to resolve. Please provide a description for this PR |
2 participants
No description provided.