Repository navigation
mpl: keep snapped macros inside the core - #11901
naveenvenk17 wants to merge 4 commits into
Conversation
Filter legal track candidates before trying them, including manufacturing-grid rounding and the oriented bounding box. Keep required-grid checks when no legal snap exists. Fixes The-OpenROAD-Project#11278 Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request updates the macro snapper in the MPL tool to ensure that macros remain within the core area during grid alignment and pin snapping. It introduces checks to verify if a macro's origin is inside the core, clamps origins to valid manufacturing grid boundaries when rounding pushes them out, and filters out illegal snapping positions. Feedback on these changes suggests adding a safety guard against division-by-zero when retrieving the manufacturing grid, optimizing the boundary calculations by using integer arithmetic instead of floating-point operations, and caching the pin offset in attemptSnapToExtraPatterns to avoid redundant function calls inside a loop.
| const int minimum_origin | ||
| = static_cast<int>(std::ceil( | ||
| lower_bound / static_cast<double>(manufacturing_grid))) | ||
| * manufacturing_grid; | ||
| const int maximum_origin | ||
| = static_cast<int>(std::floor( | ||
| upper_bound / static_cast<double>(manufacturing_grid))) | ||
| * manufacturing_grid; |
There was a problem hiding this comment.
We can avoid floating-point conversions and potential precision/performance overhead by using pure integer arithmetic to compute minimum_origin and maximum_origin.
const int minimum_origin
= (lower_bound >= 0
? (lower_bound + manufacturing_grid - 1) / manufacturing_grid
: lower_bound / manufacturing_grid)
* manufacturing_grid;
const int maximum_origin
= (upper_bound >= 0
? upper_bound / manufacturing_grid
: (upper_bound - manufacturing_grid + 1) / manufacturing_grid)
* manufacturing_grid;| std::vector<int> legal_positions; | ||
| legal_positions.reserve(total_attempts + 1); | ||
| // Keep the existing nearest-first order, but cull before moving the macro. | ||
| for (int i = 0; i <= total_attempts; i++) { | ||
| int steps = (i % 2 == 1) ? (i + 1) / 2 : -(i / 2); | ||
|
|
||
| int current_index = start_index + steps; | ||
|
|
||
| if (current_index < 0 | ||
| || current_index >= layers_data_list[0].available_positions.size()) { | ||
| continue; | ||
| } | ||
| snapPinToPosition(snap_pin, positions[current_index], target_direction); | ||
| const int position = positions[current_index]; | ||
| if (isOriginInsideCore( | ||
| getSnappedOrigin(snap_pin, position, target_direction), | ||
| target_direction)) { | ||
| legal_positions.push_back(position); | ||
| } | ||
| } |
There was a problem hiding this comment.
The getPinOffset function is called 101 times inside the loop for the same snap_pin and target_direction, which is highly redundant. We can cache the pin offset before the loop to avoid these redundant calls and improve performance.
std::vector<int> legal_positions;
legal_positions.reserve(total_attempts + 1);
const int pin_offset = getPinOffset(snap_pin, target_direction);
// Keep the existing nearest-first order, but cull before moving the macro.
for (int i = 0; i <= total_attempts; i++) {
int steps = (i % 2 == 1) ? (i + 1) / 2 : -(i / 2);
int current_index = start_index + steps;
if (current_index < 0
|| current_index >= layers_data_list[0].available_positions.size()) {
continue;
}
const int position = positions[current_index];
int origin = position - pin_offset;
alignWithManufacturingGrid(origin);
if (isOriginInsideCore(origin, target_direction)) {
legal_positions.push_back(position);
}
}|
@joaomai I'm assigning you as apparently there was already an initial round of review in a separate PR. |
Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
|
Cached the pin offset before filtering the candidate positions. All 51 MPL and repository targets passed again on the VM, followed by three shuffled runs of the 56 snapper cases. The two-file update also passed independent review. I left out the nonpositive-grid guard because the earlier maintainer review explicitly asked us to assume valid technology data. I also kept the double-based floor/ceil: it represents every 32-bit DBU coordinate exactly and handles negative bounds without the signed-overflow risk in the proposed integer formulas. |
Keep the feature tree unchanged and refresh pull-request CI after the missing registry checksum was added to master. Signed-off-by: Naveen Venkat <81929600+naveenvenk17@users.noreply.github.com>
vvbandeira
left a comment
There was a problem hiding this comment.
approved from devops team side
| lowest_grid_positions[starting_position_index], | ||
| target_direction); | ||
|
|
||
| attemptSnapToExtraPatterns( |
There was a problem hiding this comment.
This function can be renamed to something akin to "snapToGridPatterns" since the extra has no meaning now.
| if (legal_positions.empty()) { | ||
| // Keeping the original location must not bypass mandatory pin-grid rules. | ||
| totalAlignedPins(layers_data_list, target_direction, true); | ||
| return; |
There was a problem hiding this comment.
Early returning here skips the warning down the function, which is undesirable.
| const int initial_origin = origin; | ||
| alignWithManufacturingGrid(origin); | ||
|
|
||
| if (!isOriginInsideCore(origin, target_direction)) { |
There was a problem hiding this comment.
Couldn't the same be achieved by changing the way alignWithManufacturingGrid rounds the origin value? In the sense that if rounding down fails, rounding up should pass and vice-versa.
Fixes #11278.
A macro can pass the initial core check and then move outside it when its origin is rounded to the manufacturing or routing grid. This keeps the snapping step inside the core instead of rejecting the result afterward.
Snapper collects the usual nearest 101 track positions, computes their manufacturing-grid-rounded origins, and removes candidates whose oriented macro box would cross the core. It then scores only the legal candidates.
snapPinToPositiononly moves the macro; legality checking is separate. If no candidate is legal, the original position is retained and mandatory RightWayOnGridOnly pin checks still run.This follows the earlier review's requested structure. The redundant post-snap check is removed. The technology grid is assumed to be valid and positive; unrelated nonpositive-grid hardening was removed. The self-overlap fix remains separate in #11322 and is not part of this diff.
Tested on the Linux GCP VM:
Replaces #11844 under the correct contributor account. The VM-tested file tree was verified byte-for-byte before creating this PR.
CI note: upstream d61ef5a now records the missing registry checksum. No dependency versions changed. This CI refresh leaves the tested feature source unchanged.