Repository navigation
Conversation
Timing-driven placement calls Resizer::findResizeSlacks, which can add buffers through repair_design, fullyRebuffer and repair_timing. gpl only models SIGNAL and CLOCK nets, so a buffer on any other net (for example the SCAN nets execute_dft_plan creates) had no connections in the placer. Its input pin then crashed the wirelength gradient. Scan nets also should not be buffered before scan_opt reorders the chains; repair_design after scan_opt buffers them. findResizeSlacks now takes a predicate for the nets the placer models, and gpl passes its own isValidSigType rule. While it is set, okToBufferNet, the repair_design net loop and fullyRebuffer leave other nets alone. Gate sizing is unaffected. simple01-td-scan-net marks a high-fanout net SCAN and checks that it keeps all of its loads after timing-driven placement. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
fixPointers added every new pin with a GPin to its gcell, even when the pin's net has no GNet. The wirelength gradient then dereferenced the null GNet. The resizer now only buffers nets gpl places, so such a pin means gpl and rsz disagree about the net set; GPL-0335 names the pin and net. Unconnected pins are skipped, as in the initial netlist. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
![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 restricts buffering during timing-driven global placement to only the nets modeled by the placer, preventing the buffering of unplaced nets such as SCAN nets. This is implemented by passing a filtering callback to findResizeSlacks and verifying isPlacedNet during rebuffering and driver repair. The review feedback correctly identifies a potential reentrancy issue in PlacedNetGuard where nested calls could prematurely clear the outer callback, and suggests capturing and restoring the previous state of placed_net_ upon destruction.
The guard cleared placed_net_ on exit. It now restores the value it replaced, so a nested call could not drop an outer caller's predicate. Suggested in review. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
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. |
execute_dft_plan gave every port it creates sigType SCAN, including scan enable. Scan enable is a high-fanout control net, not part of a chain that scan_opt reorders, but as a SCAN net gpl does not place it and timing-driven repair_design does not buffer it. On sky130hd/ibex it reached placement as one unbuffered 1,939-pin net and was only buffered in the resize step, after global placement. The scan-enable port and net are now SIGNAL, the same as an existing port named for scan enable. Scan-in, scan-out and chain nets stay SCAN. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
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
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.
Timing-driven global placement crashes in
NesterovBaseCommon::getWireLengthGradientWAon designs with inserted scan chains (seen in ORFS on sky130hd/aes, sky130hd/ibex, gf180/ibex and ihp-sg13g2/aes withGPL_TIMING_DRIVEN=1).Cause
gpl models only SIGNAL and CLOCK nets. Pins on any other net, such as the SCAN nets that
execute_dft_plancreates, never become gpl pins. During timing-driven placement,Resizer::findResizeSlacksrunsrepair_design, which buffered a SCAN net. The callback created a GPin for the buffer's input, but the SCAN net had no GNet, sofixPointersleft that GPin in the buffer's gcell with a null net, and the wirelength gradient dereferenced it.Scan nets also shouldn't be buffered before
scan_optreorders the chains. When the chains are restitched, those buffers are left behind on the old connections.Changes
findResizeSlackstakes a predicate for the nets the placer models, and gpl passes its ownisValidSigTyperule. While the predicate is set,okToBufferNet, therepair_designnet loop andfullyRebufferleave every other net alone, so gpl and rsz work on the same nets. Gate sizing is unaffected, andrepair_designrun outside placement still buffers scan nets.fixPointersfinds a new pin on a net gpl doesn't place, it now reports GPL-0335 instead of crashing. Unconnected pins are skipped, as in the initial netlist.scan_optreorders, so gpl should place it and timing-driven placement should buffer it. Scan-in, scan-out and chain nets stay SCAN.simple01-td-scan-net: marks a high-fanout net SCAN, runs timing-driven placement, and checks that the net keeps all of its loads.