Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grt: Jumper insertion improvement #6627

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

luis201420
Copy link
Contributor

New implementation of jumper insertion using DFS algorithm to find the GSegments that cause antenna violation for a pin.

The number of inserted jumpers increases because a jumper will be added to each GSegment connected to the pin with violation.

In the results obtained the number of diodes inserted in the post-GRT stage in most designs is reduced.

@luis201420 luis201420 changed the title Grt: jumper insertion improvement Grt: Jumper insertion improvement Jan 31, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

src/grt/src/RepairAntennas.cpp Show resolved Hide resolved
src/grt/src/RepairAntennas.h Outdated Show resolved Hide resolved
src/grt/src/GlobalRouter.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Feb 3, 2025

clang-tidy review says "All clean, LGTM! 👍"

@eder-matheus
Copy link
Contributor

@luis201420 CI have an error:
The following tests FAILED: grt.repair_antennas4.tcl (Failed)

mterm->getConstName(),
mterm->getMaster()->getConstName());
return pin_name;
return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the function always return true, why returning something at all? Should it be a void instead of bool?

@@ -1062,8 +926,8 @@ void RepairAntennas::addJumperHorizontal(const int& seg_id,
route[seg_id].init_x = bridge_final_x;
}

void RepairAntennas::addJumperVertical(const int& seg_id,
GRoute& route,
void RepairAntennas::addJumperVertical(GRoute& route,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functions addJumperVertical and addJumperHorizontal looks almost like the same. Perhaps you could merge them into a single function that receives a bool to define if the jumper is vertical or horizontal, and then you can define the jumper coordinantes according to this boolean argument.

mterm->getConstName(),
mterm->getMaster()->getConstName());
return pin_name;
return true;
}

int RepairAntennas::getSegmentByLayer(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better name would be getSegmentsPerLayer.

Comment on lines +1082 to +1085
void RepairAntennas::getSegmentsConnectedToPin(
const odb::dbITerm* iterm,
LayerToSegmentNodeVector& segment_by_layer,
SegmentNodeIds& seg_connected_to_pin)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it would be easier to use the GlobalRouter::Pin here, instead of the odb::dbITerm. With GlobalRouter::Pin, you can simply use the getOnGridPosition function and use it to check what segments overlap with the pin or not.

int pos_y = seg_init_y;
int last_block_x = pos_x;
int last_block_y = pos_y;
bool has_available_resources, is_via;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid multiple declarations in a single line. Tho it saves line count, I find it quite easy to miss one of the declarations.

getViaPosition(segment_graph, route, seg_node, via_pos);

// Save best positions to add jumper
std::vector<int> position_cand;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does cand stand for?

Copy link
Contributor

github-actions bot commented Feb 3, 2025

clang-tidy review says "All clean, LGTM! 👍"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants