Repository navigation
Conversation
|
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}
There was a problem hiding this comment.
Code Review
This pull request introduces the experimental PARALLEL_EVAL setup repair policy, which optimizes setup timing in parallel batches. To support this, several new components are added, including SetupParallelEvalPolicy, BatchSelector, BottleneckAnalysis, SequentialTimingEstimator, and RepairSetupProfiler, while older experimental policies like MeasuredVtSwapPolicy and SetupMt1Policy are removed. Additionally, DelayEstimator and Rebuffer are refactored to support multi-threaded planning, caching, and side-load timing estimation. The code review feedback highlights several critical safety issues across these new files, primarily concerning potential null pointer dereferences (such as missing null checks on stage.arc.scene, target_arc.inputPort(), plan.targetPath(), plan.loadPath(), edge->role(), and inst pointers) and a potential crash in RebufferPlannerPool if the idle pool is empty during acquisition.
17b786c to
131e67e
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
What: the Mt delay estimator models the fanin side loads, the opposite output transition, the latch D-side driver and the flip-flop setup and hold change of a cell swap, and SizeUpMt and VtSwapMt charge them. Why: scoring a swap on the selected path alone let a scored gain turn into a loss after the timing update. How: side models in DelayEstimator and SequentialTimingEstimator, input cap budgets that workers can check without STA, and ArcDelayState renamed to EstimatorContext. The side models are off by default. Setup repair profiler review follow-ups: the profiler constructor takes a RepairSetupProfiler::Mode instead of an int, CRIT_VT_SWAP samples WNS and TNS before its sweep, and the Bazel TestResizerMt target drops the --gtest_filter that skipped the profiler and fixture tests. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: RebufferMt, an Mt buffering move that plans a beam of Rebuffer trees per driver and scores each with the buffer-tree estimator. Why: buffering needs the same scored, comparable candidates as the cell swaps before a batch policy can choose among them. How: makeRebufferTreePlans() and applyRebufferTreePlan() in Rebuffer, estimateBufferTreeWithSidePaths() in DelayEstimator, and current required times after the endpoint scan. No policy runs RebufferMt yet; PARALLEL_EVAL will. LEGACY is unchanged. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
What: PARALLEL_EVAL, an experimental setup repair policy that commits a batch of VtSwapMt, SizeUpMt and RebufferMt moves per timing update. Why: one timing update per batch instead of per move cuts the repair time. How: worker threads scan endpoints and generate and score candidates from prepared data; BatchSelector, BottleneckAnalysis and ParallelEvalState pick, rank and stop; each batch is kept, salvaged or rolled back by its WNS and TNS (the group's under -path_group). OptimizationPolicy::finish() ends each phase, and the Optimizer prints the final report after the last phase. LEGACY results do not change. Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>
131e67e to
32b4e4d
Compare
|
Claude == Priority 1 (correctness) ==
|
| } | ||
| } | ||
|
|
||
| constexpr size_t kBitsPerWord = 64; |
There was a problem hiding this comment.
This should be a sizeof for the relevant type (uint64_t I think).
| return id < node_of_.size() ? node_of_[id] : -1; | ||
| } | ||
|
|
||
| std::vector<size_t> BottleneckAnalysis::collectCone( |
There was a problem hiding this comment.
After discussing w/Codex:
There is a narrower caveat: the cone’s filter does not fully match OpenSTA’s traversal rules. OpenSTA also excludes some edges, such as disabled bidirectional instance paths and, by default, preset/clear arcs, when assigning vertex levels. collectCone can include those edges, so its assumption that every included fanin has a lower level is not guaranteed in those cases. That could distort the bottleneck weights; it does not negate the handling of edges explicitly marked as broken loop edges. Levelize.cc (src/sta/search/Levelize.cc:188)
| // Failing endpoints at the last collection and the last saved checkpoints. | ||
| size_t fep_{0}; |
| // Strength of the cone-need bonus. | ||
| float alpha{2.0f}; | ||
| // Emphasis on the most shared bottlenecks. | ||
| float gamma{2.0f}; |
There was a problem hiding this comment.
More meaningful names than alpha/gamma
| float work_sta_per_vertex{5.0e-7f}; | ||
| float work_per_pending_net{1.93e-3f}; | ||
| float work_per_estimate{1.35e-5f}; | ||
| float work_per_plan{1.14e-3f}; | ||
| float work_per_endpoint{5.65e-4f}; |
There was a problem hiding this comment.
Where do these values come from?
2 participants
Part 6 of 12 of the PARALLEL_EVAL setup repair stack. It builds on #11798 and also shows the commits of parts 1 to 5; its own change is the last commit.
Summary
PARALLEL_EVAL, an experimental setup repair policy that commits a batch of moves per timing update instead of one move per pass. It is the first policy that runs the Mt moves: VtSwapMt, SizeUpMt and RebufferMt here, the other Mt moves in the next PRs.Changes
BatchSelectorpicks a conflict-free batch by score and reserves room for the worst endpoints when WNS stalls;BottleneckAnalysisgives the drivers that many failing endpoints share a higher weight.-path_groupthe check reads the group's WNS and TNS.ParallelEvalStateholds the work meter, which counts work instead of wall time, the stall monitor, the WNS tail of small batches on the worst endpoints, and the miss tabu.-effort low|medium|highsets the stop rules,-max_iterationscounts batches, and a batch that would leave the design above-max_utilizationis undone.RebufferMtGenerator; their warnings are counted and reported once (RSZ-2034).Optimizer::run()callsOptimizationPolicy::finish()at the end of each phase, so PARALLEL_EVAL releases its threads before the next phase, and prints the final report itself after the last phase (reportRunResult()), so each policy ends with its phase.flowchart TD A["scan the violating endpoints<br/>(worker threads)"] --> B["collect targets in chunks,<br/>worst slack first"] B --> C["prepare the STA reads<br/>(main thread)"] C --> D["generate and score candidates<br/>(worker threads)"] D --> E["select a conflict-free batch<br/>by score"] E --> F["commit the batch in one journal,<br/>then one RC and STA update"] F --> G{"WNS or TNS better<br/>than the checkpoint?"} G -- yes --> H["save the checkpoint"] G -- no --> I["revert the culprits,<br/>else roll back"] H --> J{"stalled?"} I --> J J -- no --> A J -- yes --> K["WNS tail: small batches<br/>on the worst endpoints"]Behavior
PARALLEL_EVALtoken of-phases; LEGACY results stay the same.Stack