Repository navigation
rsz: Address PARALLEL_EVAL review feedback - #11905
Merged
Merged
Conversation
What: the switches this stack added on BufferedNetType (Rebuffer.cc) and on MoveType (SetupParallelEvalPolicy::generatorAllowed()) list every enumerator and have no default label. Why: review of The-OpenROAD-Project#11798. With a default label the compiler does not warn when a new enumerator is added, so the code that needs a decision for it is not found. How: replace each default label with the missing cases. A function that returns a value keeps a return after the switch, as BufferedNet.cc does. The three "unhandled BufferedNet type" errors RSZ-1009, RSZ-1011 and RSZ-1012 go with their labels. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: profile mode 1 writes metadata.csv, stats.csv and timing.csv; events.csv starts at mode 2, as the README and architecture.md say. Why: review of The-OpenROAD-Project#11797. Mode 1 opened events.csv and wrote only its header, because emit() writes rows only from mode 2, so the mode 1 output had an empty file the documentation does not list. How: open events.csv and write its header only when the mode is at least Mode::kEvents. The two profiler unit tests check that mode 1 leaves no events.csv and read the trace in mode 2 only. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: SlewBiasModelKey hashes with a nested Hash functor whose operator() is defined in DelayEstimator.cc, instead of an inline friend hash_value() in DelayEstimator.hh. Why: review of The-OpenROAD-Project#11797. The hash was too big for a header. How: declare SlewBiasModelKey::Hash in the header and use it as the hasher of SlewBiasModelMap, move the body unchanged to the .cc, and move the boost/functional/hash.hpp include with it. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: the slew-bias cache key of DelayEstimatorCache and the buffer delay cache key of Rebuffer hold their slews, caps and Pi model values as floats, not as the bits of those floats. Why: review of The-OpenROAD-Project#11797. The caches only need equal values to reuse a sample, so the bit copies added code without need. Equal floats hash alike with boost::hash, which also treats +0.0 and -0.0 as equal; the keys never hold a NaN. How: replace the uint32_t fields with float fields, drop floatBits() and the float_bits lambda, and hash the floats with boost::hash_combine. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: InputCapBudget stores the smallest load increase any driver of the input net still accepts, instead of a list of DriverCapLimit with three fields per driver. Resizer::maxCapIncrease() returns that headroom for one driver, and checkMaxCapOK() compares the cap delta with it. Why: review of The-OpenROAD-Project#11797. negative_slack only served to pick the limit at the comparison, so the budget can store the result. Doing the live check the same way keeps the two answers equal at the float boundary, where cap + delta > limit and delta > limit - cap can differ. How: maxCapIncrease() is 0 for a violating driver, max_cap - cap with headroom and infinity without a limit; inputCapBudgets() keeps the minimum over the drivers. A unit test checks that the budgets and replacementPreservesMaxCap() agree with a tight limit and with a violated one. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: batch_candidates_ and batch_swaps_ are cleared at the end of SetupParallelEvalPolicy::iterate(), and resetRunState() clears batch_swaps_ too. Why: review of The-OpenROAD-Project#11799. Both vectors point into the BatchPool local to iterate(), so they dangled until the next batch cleared them. Nothing reads them after iterate() today; clearing them keeps it that way. How: clear both vectors after the stop rules and the batch summary, and say in the member comment that they belong to the pool. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: collectViolatingEndpoints() calls findRequireds() before it branches on -path_group, as RebufferMt had it, not only in the design-wide branch. Why: review of The-OpenROAD-Project#11799. RebufferMt planners on worker threads read required times without a refresh (annotateLoadSlacks()). Under -path_group they were current only because main-thread slack queries for the side loads happened to update them first. How: move the findRequireds() block above the branch and say in its comment which workers depend on it. It is a no-op when timing is up to date, so results do not change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: RebufferPlannerPool::acquire() raises RSZ-2036 when no planner is idle, instead of calling back() and pop_back() on an empty vector. Why: review of The-OpenROAD-Project#11799. The pool holds one planner per worker thread, so it cannot run dry today, but nested pool work inside planning would take a second lease on a thread and hit undefined behavior. How: check idle_ under the lock and call logger()->error(), which throws; parallelFor() rethrows on the main thread and no lease is taken. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: an undone PARALLEL_EVAL batch puts on the tabu only the instances that its journal rollback kept, for the batch targets and for the instances of a failed speculative run. Why: review of The-OpenROAD-Project#11799. In the WNS tail a speculative run keeps the journal open, so a later batch can target a buffer the run created. The rollback deletes that buffer, and OpenDB gives its address to the next instance it creates; that instance started out on the tabu, or excluded after enough undos. How: commitBatch() records each target instance with its OpenDB name, read before the move, and the speculative run keeps those records. After the rollback, keptInstances() keeps an instance only when an instance of the recorded name is still at the recorded address; the recorded pointers are compared, never dereferenced. The golden logs do not change; a unit test deletes an instance and reuses its slot. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: the comment of size_up_mt_score_scale says that 0.75 comes from an Fmax sweep and what it does. Why: review of The-OpenROAD-Project#11797 ("why .75?"). The value had no explanation in the code. How: comment only. The scale was set by an Fmax sweep when SizeUpMt and VtSwapMt started to share one score pool; below 1 it lets a VT swap win over a size-up of similar gain. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: the comment above the buffer-tree calibration factors names the data set they were fitted on and what an off factor costs. Why: review of The-OpenROAD-Project#11798. It was not clear which data the accuracy report was run on, or whether the factors hold on other PDKs. How: comment only. The factors come from the Nangate45 cases of report_delay_estimator_accuracy_for_rebuffering (the report_delay_estimator_rebuffer_* tests), and no other platform was checked; the estimate only ranks trees, and STA times each batch before it is kept. The comment also gets the report command name right. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: boundBufferTreeDownstreamStageDelay() says what it bounds and why. Why: review of The-OpenROAD-Project#11798, which asked whether the floor works around a bad slew estimate. It does: the tree model's wire slew is approximate, and an optimistic slew at the inputs after the tree made the table delta credit those stages with too much delay gain. How: comment only. A stage after the tree keeps 80% of its current delay when its input slew does not improve, falling to 50% as the slew gets sharper. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: the comment of kBufferTreeHighCapSlewRatio and kBufferTreeHighCapSlewFactor says why a heavily loaded final buffer takes its output slew at a lower effective cap. Why: review of The-OpenROAD-Project#11798, which asked why this correction is needed. It has the same cause as the effective-cap factors above it: the Liberty lookup uses a lumped cap, while STA's delay calculator drives an RC load with an effective cap below it, and the wire resistance hides the most cap at a final buffer that drives a long load. How: comment only. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: the header of each repair_setup_path test says which platform and design the path slice comes from, how many endpoints violate, which LEGACY moves the golden log covers and where its worst path runs. Why: review of The-OpenROAD-Project#11794. Every header ended with "Drives LEGACY repair_setup.", which said nothing about what the case adds. How: comments only; the facts come from each test's golden log. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: the TestResizerMt fixture no longer redefines "private" or "protected" around any header. The tests reach the internals of SetupParallelEvalPolicy, StallMonitor, WnsTail, SizeUpMtGenerator and RepairTargetCollector through ResizerTestPeer, which those classes befriend, and the three pure batch rules of SetupParallelEvalPolicy are public. Two flip-flop tests attach OpenSTA scaled models through ScaledTimingArc and ScaledLibertyPort. Why: review of The-OpenROAD-Project#11797. Redefining an access keyword before including a header is undefined behavior: it can change the class layout and break the one-definition rule. How: one friend declaration per class; ResizerTestPeer in the fixture forwards each internal the tests read or set, and the tests call it. adaptiveBatchSize(), evaluationEndpointLimit() and batchImprovesCheckpoint() move to the public part of the policy. The redefinition around tst/IntegratedFixture.h was not needed. TimingArc::addScaledModel() and LibertyPort::addScaledPort() stay protected: a class derived from each takes a pointer to the member, which C++ allows, so OpenSTA is unchanged. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: salvageMissedBatch() counts a timing update before it rolls the batch back, as undoBatch() and restoreBatchCheckpoint() already do. Why: review of The-OpenROAD-Project#11799. MoveCommitter::restoreJournal() updates parasitics and timing after it restores ECO edits, so the salvage rollback is real STA work. The work meter left it out, so the stall window and the tail budget saw less work than the run did. How: call noteTimingUpdate() before rollBackJournal() and fix the comment there and the work-meter doc in OptimizationPolicyConfig.hh. WNS, TNS, instance count and area stay the same on 34 designs. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: comments in BatchSelector::admit(), the batch selection section of architecture.md and the BatchSelector dependency test say why a candidate is admitted when it changes timing that a member's estimate read. Why: review of The-OpenROAD-Project#11799 asked why one direction is enough. Every estimate reads the same pre-batch timing, so either direction leaves a stale estimate, and the measured batch verdict and the salvage catch a batch that misses. Checking both ways was measured on 34 designs: average Fmax +0.1%, but 10 designs better and 14 worse; TNS better on 11 and worse on 6. The test comment called impact3 "later", which read as if the order made the one-way check safe. How: comments and docs only. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: kBitsPerWord in BottleneckAnalysis.cc is sizeof(uint64_t) * CHAR_BIT instead of the literal 64. Why: review of The-OpenROAD-Project#11799. The reach sets are vectors of uint64_t, and the literal did not say that it is the bit width of that type. How: compute the constant from the word type. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: the bottleneck cone of PARALLEL_EVAL no longer follows the preset/clear arcs OpenSTA disables by default, or disabled bidirect instance paths. Why: review of The-OpenROAD-Project#11799. The cone sums run in level order and assume that every fanin has the lower level. Levelize::searchThru() skips these edges, so a vertex reached only through one can sit above its fanout: a reset pin RN can have a higher level than Q. The fanout sums then hand Q's endpoints to RN after RN passed its own set on, and the cone weights are wrong. How: forEachDataFanin() also skips a regSetClr arc while preset/clear arcs are disabled and an edge isDisabledBidirectInstPath() reports. A new unit test connects RN of a DFFR_X1 to a deep net: without the fix RN collects the three endpoints behind Q. The rsz regression goldens do not change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: fep_, SavedCheckpoint::fep and the fep_count parameters of the PARALLEL_EVAL stall monitor and batch cap are named after the failing endpoint count they hold. Why: review of The-OpenROAD-Project#11799 asked to avoid cryptic names such as fep_. How: rename the members, parameters, locals and test variables; the profiler event batch_fep_at_collect keeps its name because reports read it. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: BatchPriorityConfig::alpha and gamma are now cone_bonus_scale and cone_bonus_exponent, in the code, the tests and architecture.md. Why: review of The-OpenROAD-Project#11799 asked for more meaningful names than alpha and gamma. The rank is score * (1 + cone_bonus_scale * w^cone_bonus_exponent), so the names say which part of the bonus each one sets. How: rename the two fields and update the formula comments and the field table. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: the work-meter comment in OptimizationPolicyConfig.hh says the work_* weights are a non-negative least-squares fit of profiled batch wall times on the reference machine. Why: review of The-OpenROAD-Project#11799 asked where these values come from. How: comment only. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: a comment on kSplitLoadMinFanout in SplitLoadMtGenerator.cc says it is the fanout threshold of the LEGACY split-load move. Why: review of The-OpenROAD-Project#11800 asked why 8. SplitLoadGenerator, and split_load_min_fanout_ before it, uses the same value, so both policies consider the same drivers. How: comment only. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: SplitLoadMtCandidate::apply() finds the output pin of the new buffer once, before the resize, and uses it for the resize and for the parasitics of the new net. Why: review of The-OpenROAD-Project#11800. The pin was looked up again after the resize; that works because dbInst::swapMaster() keeps the instance terminals, but one lookup is clearer. How: hoist bufferOutputPin() into a local. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: a comment on kBufferRemovalMaxFanout in UnbufferMtGenerator.cc says it is the fanout limit of the LEGACY buffer removal, used when the upstream driver has no max-fanout limit of its own. Why: review of The-OpenROAD-Project#11802 asked why 10. UnbufferGenerator uses the same value, which came with the buffer removal move in 2024. How: comment only. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: the comment on WnsTail::unbufferAllowed() explains why a salvage cannot undo a buffer removal on its own. Why: review of The-OpenROAD-Project#11802 asked why a removal cannot be reverted in place. A cell swap goes back with one replaceCell() and every pin and net stays the same object. A removed buffer and its net come back only through the journal restore, which undoes the whole batch. How: comment only. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: MissTabu entries keep the name of the instance they were recorded for, and a lookup counts an entry only when the live instance at that address still has that name. A new instance at a reused address starts with no tabu, no undo count and no exclusion. Why: review of The-OpenROAD-Project#11802. A kept batch can delete an instance that has a tabu entry: a buffer removal, or a buffer a rebuffer replaced. OpenDB hands the freed slot to the next new instance, which then inherited the old entry and could be skipped, or excluded for the rest of the run. A rollback is not affected: the journal restores a deleted instance at its old address with its old name, so the undone buffer keeps its tabu and its undo count. How: an Entry of name and value for each map, filled through the dbNetwork MissTabu::reset() now takes; isTabu(), isExcluded() and recordUndo() compare the name and read only the live instance. A new unit test deletes a tabu'd instance, creates one in its slot and checks that the new one is free. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: UnbufferMtCandidate::impact() marks as reconnected the pins of the net removeBuffer() drops: the output net's loads, or the input net's pins when only the output net has a port. Resizer gains removeBufferKeepsOutputNet(), which reports that choice. Why: review of The-OpenROAD-Project#11802. The impact always marked the output net's pins, but removeBuffer() keeps a port net and moves the pins of the input net onto it. A move on the upstream driver then showed no reconnect conflict, so a multi-move batch could admit both. Only batches with unbuffer in -sequence hold a removal next to other moves. How: removeBufferKeepsOutputNet() applies the rule removeBuffer() and canRemoveBuffer() use, bufferRemovalKeepsOutputNet(). A new unit test checks both cases on the test netlist. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: when the upstream driver has no capacitance limit, UnbufferMt runs its slack guard and gain estimate on the target path's scene. Why: review of The-OpenROAD-Project#11802. The guard used the scene of the driver's worst capacitance check, which is null without a limit, and estimatedSlackOK() rejects every removal on a null scene. So UnbufferMt never removed a buffer behind a driver without a limit. How: fall back to the path scene that resolveDriverContext() already holds. The LEGACY UnbufferGenerator has the same gap and is unchanged. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: Resizer::removeBuffer() takes the buffer's area off the tracked design area that overMaxArea() reads, and PARALLEL_EVAL judges -max_utilization with overMaxArea() instead of walking the area. Why: review of The-OpenROAD-Project#11803. Every insertion and resize updates the tracked area, but a removal left it unchanged, so overMaxArea() saw the area of removed buffers. LEGACY's stop rules and the final checks read it. PARALLEL_EVAL worked around this with an instance walk per edit, and rollBackJournal() recounts the area after an undo already. How: designAreaIncr() with the negative master area before both removal paths delete the buffer. withinMaxUtilization() returns !overMaxArea(); the walk stays only for the RSZ-2033 message. The unit test of a batch with a removal and an upsize now checks that the tracked area stays below the limit. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: RepairSetupAnalysis::windowDelay() searches the paths of the pin where the captured path ends, instead of the original endpoint. Why: review of The-OpenROAD-Project#11804. The window chain is captured from the target's endpoint path, which ends at the target driver for a driver prefix and at a latch D pin for a latch fanin segment. windowDelay() looked for that chain among the paths of the original endpoint, never found it, and left window_delay_s, actual_window_gain_s and estimate_error_s empty without a warning. How: look up the vertex of path_points_.back(). A new unit test builds a driver-prefix target and checks that its window delay is sampled; it fails before the fix. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: report_delay_estimator_accuracy_for_rebuffering errors out with RSZ-3211 when -driver_pin is not an output pin of -inst. Why: review of The-OpenROAD-Project#11804 (Claude and Codex). The pin was used as given: with -inst r2 -driver_pin r1/Q the report analyzed r1/Q under the header of r2, and an input pin of -inst was taken as its driver. The instance also feeds the estimator context, so the comparison was misleading. How: reportAccuracyForRebufferTree() checks that the pin belongs to the instance and is a driver. The new report_delay_estimator_rebuffer_ driver_pin test covers both cases and is registered in Bazel and CMake. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: the ECO scope of the delay estimator accuracy reports prints warning RSZ-3212 when its undo or the timing update after it throws. Why: review of The-OpenROAD-Project#11804. The destructor caught every exception and returned quietly, so a failed undo left the report's trial buffers or cell swap in the design with no message. How: keep the catch, since a destructor must not throw, and warn in it. No change when the undo succeeds. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: Resizer::repairEffortFromString() raises RSZ-3242 for an unknown effort and Resizer::repairSetup() raises RSZ-3243 for a profile mode outside 0-3. The Tcl range checks are gone; Tcl still checks that -profile_mode is an integer and keeps its two warnings. Why: review of The-OpenROAD-Project#11804 asked for such validation in C++ so it covers every API. The Python binding reached repairSetup() without either check: an unknown effort became high, and a mode past 3 reached the profiler, which no longer clamps it. How: repairEffortFromString() is a const member now, so it can log; the Tcl and Python bindings call it on the Resizer. The effort unit test also checks the error, and the new repair_timing_option_errors test, registered in Bazel and CMake, checks both messages from Tcl. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: the class that captures timing and topology around one move, and its RepairSetupAnalysis.hh/.cc files, are now RepairSetupMoveSampler. Why: review of The-OpenROAD-Project#11804. The class does not analyze anything; it samples the state before and after a move, which the report then analyzes. How: rename the class, the files, the includes, the Bazel and CMake source lists and the architecture.md references. The report, the analysis target and the profiler mode keep their names. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: RepairSetupAnalysisData.hh, which holds the snapshot types of the move sampler, is now RepairSetupMoveSnapshot.hh, and RepairSetupAnalysisSnapshot.cc, which captures those snapshots, is now RepairSetupMoveSnapshot.cc. Why: review of The-OpenROAD-Project#11804, following the RepairSetupMoveSampler rename: the files hold the state captured around a move, not an analysis. How: rename the files, the includes, the Bazel and CMake source lists and the architecture.md reference. The types keep their names. No behavior change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: RebufferMtGenerator drops a tree plan that puts a dont-touch load behind a buffer, so no candidate is made from it. Why: review of The-OpenROAD-Project#11798 (Codex). The tree export skips a dont-touch load and leaves it on the driver net, so the committed tree could differ from the scored one while apply() accepted any positive buffer count. Otherwise the export builds the scored tree: insertBufferBeforeLoads() fails without an error only on a dont-touch net or load, and okToBufferNet() already keeps a dont-touch driver net out. Dropping the tree at generation keeps it out of scoring and batch selection. How: makeTreePlans() adds a plan gate next to the one-buffer and score-cap gates; a tree that keeps the dont-touch load on the driver net stays. RebufferMtCandidate.hh joins the headers the tests see. A unit test marks a load that a planned tree puts behind a buffer as dont-touch and checks that no candidate puts it there. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: the comment of fanin_side_loss_weight says that the weight of 3 is calibrated and how. Why: review of The-OpenROAD-Project#11797 ("Why 3?"). The value had no explanation in the code. How: comment only. Applied moves whose estimated fanin side loss reached about a quarter of their gain did not improve the measured TNS, and a weight of 3, which leaves no gain once the side loss reaches a third, fit best. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Keep exhaustive switches without default labels so new enum values receive compiler diagnostics. Restore the Rebuffer message IDs after the switches and add logger failures for the helper and generator gate when an unhandled value reaches runtime. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Replace the remaining logic_error throws in parallel evaluation salvage with RSZ logger errors. This keeps error propagation while reporting failures with stable RSZ message IDs and removes the unused stdexcept include. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Rename the slack-deficit, bottleneck-severity, and priority-scale identifiers to reflect their actual roles in stalled-batch ranking. Keep the scoring formula and defaults unchanged while updating tests and architecture documentation. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: Remove the special final-buffer slew lookup and update the two affected rebuffer accuracy goldens. Why: Two repeated 34-design PARALLEL_EVAL comparisons preserved Fmax, WNS, TNS, and FEP, while one buffer was avoided in hercules_is_int. Clean runtime runs differed by less than one percent overall. How: Keep the existing path effective-load lookup for buffer delay and slew; delete the second high-cap lookup and its constants. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com> (cherry picked from commit 5dc99ce1ea15dae3a06e996698c458be1b53f3b5)
What: Compare the stored tabu name against the live OpenDB constant name on a map hit. Why: dbInst::getName returns a std::string copy, which is unnecessary for lookup comparisons on long instance names. How: Use getConstName for matching while retaining owned string snapshots in tabu entries. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: the comments on the downstream delay floor, the fanin side loss weight, the SizeUpMt score scale and the one-way batch dependency check give their rationale instead of tuning outcomes. Why: review of The-OpenROAD-Project#11797, The-OpenROAD-Project#11798 and The-OpenROAD-Project#11799. The floor comment named a cause that the calibration data does not show, and the other three described tuning runs rather than the reason, unlike the replies posted on those PRs. How: comment only. The floor is a safeguard against crediting the stages after the tree with more gain than the built tree gives; the side loss weight keeps out moves that degrade side paths; a size-up ranks below a VT swap of similar gain because it costs more; a two-way dependency check would defer more candidates. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
Contributor
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the parallel evaluation setup and bottleneck analysis in the resizer tool. Key changes include renaming RepairSetupAnalysis to RepairSetupMoveSampler, updating the bottleneck priority configuration to use deficit-based bottleneck severity, and refactoring MissTabu to track instances by name to prevent issues with reused memory addresses. Additionally, float bitwise caching was replaced with standard floats and custom hashes, and option validation for -effort and -profile_mode was moved to C++. The review feedback identifies critical null pointer dereferences in SetupParallelEvalPolicy::commitBatch, MissTabu::name, and MissTabu::hasEntry when handling null instances (such as top-level ports), which should be guarded.
…el-eval-reviews-master Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
openroad-ci
force-pushed
the
secure-apply-parallel-eval-reviews-master
branch
from
October 21, 2026 17:49
5ff1b78 to
4f6afc9
Compare
Contributor
Author
|
@maliberty Applied your review feedbacks. |
maliberty
approved these changes
Oct 22, 2026
| // on the rebuffer accuracy report, | ||
| // report_delay_estimator_accuracy_for_rebuffering, which prints the estimate | ||
| // of every stage of the planned tree next to the delay STA measures after the | ||
| // tree is built. The data set is the Nangate45 cases of that report, the |
Contributor
Author
There was a problem hiding this comment.
Definitely, not sufficient. I'll check and enhance the accuracy further with other PDKs.
Member
|
Thanks. My only concern is when I see constants like constexpr int kSplitLoadMinFanout = 8; I was asking more about why it is 8 than what it means. How are the various magic constant's values arrived at (ie how could someone else reproduce those numbers)? |
Contributor
Author
The various constants related to PARALLEL_EVAL policy were empirically tuned with small set of test cases within a logically valid range. The |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feedback and responses
Original PR #11794
Drives LEGACY repair_setup.is pretty vague and used for every test.”Original PR #11796
phase_beforeis sampled before the sweep. ef64398ca1.intand notMode?”Mode. ef64398ca1.Original PR #11797
std::bit_castchecks the bit size.It is to reduce the overall timing QoR degradation by side-path impact. Smaller
fanin_side_loss_weightcan allow more aggressive moves that degrade side paths, which results in worse QoR.VtSwap weight is 1. Upsizing has more cost than VtSwap (need more area, increase input pin cap, can cause displacement by legalizer, can hurt side-path timings, ...), so it has lower weight than Vt swap.
Original PR #11798
Rebuffer for large fanout net has runtime overhead, thus SplitLoad move covers the
fanout > 20cases.In the
8 < fanout <= 19cases, both SplitLoadMt and RebufferMt move candidates are generated and compete.> 1problem.”I will check its usefulness again, and remove it if not necessary anymore.
reply 2: Finished the experiment.
This logic is still useful. It can be removed later if the estimation accuracy improves further.
reply 2: Removed this logic since it is not that useful.
- It is better to have the
area vs timing, orpower vs timingtrade-offs. I will enhance that part for power-aware timing repair.Original PR #11799
*is a phase marker, not a saved checkpoint; a stricter rule worsened 2nm QoR, so no change.sizeoffor the relevant type (uint64_t I think).”- Used a non-negative least-squares fit of the wall time (seconds) of 3k profiled batch samples on a reference machine.
- We cannot use the actual elapsed time. The work meter is to provide a deterministic estimated time.
Original PR #11800
findTargetCell(); the load model differs. No change.Original PR #11802
It looks like a conservative guard not to create a new DRC (or ERC) violation.
We cannot undo a specific move in the middle of a batch.
But VtSwap or SizeUp moves can be easily reverted by performing a reverse VtSwap or Sizing moves.
We cannot do that for unbuffer move. It requires a journal undo. So, the limitation (
removes buffers only in single-move batches) has been added.Original PR #11803
-setup. Skipping flops hurt QoR. No change.Original PR #11804
Original PR #11805