optimize repair_design runtime (#9502) and init cloning infra#9651
optimize repair_design runtime (#9502) and init cloning infra#9651Divinesoumyadip wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request aims to optimize repair_design runtime by replacing a global parasitic update with more localized ones, which is a valuable performance improvement. The changes in RepairDesign.cc are mostly correct, but there is a critical issue where a variable is used out of scope, which will lead to a compilation failure. Additionally, the new draft file GateCloning.cpp contains a couple of minor issues like an undefined variable and a magic number that should be addressed as the feature is developed.
Note: Security Review is unavailable for this PR.
| @@ -409,7 +409,7 @@ void RepairDesign::repairDesign( | |||
| fanout_violations, | |||
| length_violations); | |||
| } | |||
| estimate_parasitics_->updateParasitics(); | |||
| estimate_parasitics_->ensureWireParasitic(drvr_pin); | |||
There was a problem hiding this comment.
The variable drvr_pin is not defined in this scope, which will cause a compilation error. This call is outside the main for loop that processes drivers. Given that localized parasitic updates are now performed inside the loop (via repairDriver -> repairNet), this line appears to be both incorrect and redundant. It should probably be removed. The est::IncrementalParasiticsGuard will handle any final updates when it goes out of scope at the end of the block.
| Point centroid1 = calculateInitialCentroid(sinks, Partition::Left); | ||
| Point centroid2 = calculateInitialCentroid(sinks, Partition::Right); | ||
|
|
||
| for (int i = 0; i < 10; ++i) { // Iterative refinement |
There was a problem hiding this comment.
Using a magic number like 10 for the number of k-means iterations is not ideal for maintainability. It would be better to define this as a named constant, for example const int kKMeansIterations = 10;, at the beginning of the function. This improves readability and makes it easier to tune this parameter in the future.
References
- Define tunable parameters as named constants instead of using hardcoded magic numbers.
| // 3. Transformation Phase | ||
| // Duplicate driver, remap partitioned sinks, and trigger legalization | ||
| Instance* clone_inst = db_network_->copyInstance(drvr_inst); | ||
| remapSinks(clone_inst, cluster2_sinks); |
fd5a9a4 to
e6b6ac0
Compare
|
Please keep the performance optimization and any work on a new move separate. Your proposed move looks similar to CloneMove.cc. Have you done any work verifying the QoR on ariane133 and/or other designs does not degrade? |
…nit cloning infra Signed-off-by: Divinesoumyadip <soumyadipdasmahapatra343@gmail.com>
e6b6ac0 to
efb80ed
Compare
Hi @povik, appreciate the review. I've amended the commit to drop the cloning draft, keeping this PR strictly scoped to the repair_design runtime optimization.On the QoR front, this patch mathematically preserves timing accuracy. By shifting from a global |
Hi @povik, Build #2 has successfully passed the rsz regression suite in the Jenkins GCP environment. Specifically, //src/rsz/test:repair_design1-tcl_test passed in 0.3s.While this unit test is too small to demonstrate the full scalability of the |
|
@povik Can u check it out now?Thanks in advance . |
|
I have checked out the changes but to review I need your answer to
|
Note on GSoC 2026: This branch also contains the initial, unlinked
GateCloning.cppinfrastructure file, which I am staging for my GSoC proposal focusing on timing-driven spatial partitioning.cc: @maliberty @LucasYuki @povik