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

dpl: support LEF58_CELLEDGESPACINGTABLE #6595

Merged

Conversation

osamahammad21
Copy link
Member

Linked to #3899

@osamahammad21
Copy link
Member Author

osamahammad21 commented Jan 26, 2025

There will be a followup PR for optimizing performance of the checkEdgeSpacing function. This is just an initial attempt. The followup PR would include the following:

  • Instead of using the raw odb edge spacing table entries, I will create a 2d matrix representing the spacing table.
  • In this PR, a cell may be checked multiple times of it occupies more than one pixel in the search area. I will avoid that.
  • Instead of doing a separate search for each edge spacing, I will do one with the maximum spacing value for that edge.
  • I can create an rtree for edges to use for the search instead, but this would have a memory impact as each cell would have its associated edges. I will hold on this until I try with a real design test case.

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

@maliberty
Copy link
Member

I can create an rtree for edges to use for the search instead, but this would have a memory impact as each cell would have its associated edges. I will hold on this until I try with a real design test case.

I don't think this is necessary. The search on the pixel grid is already fairly efficient. The extra memory cost versus performance feels heavy.

Signed-off-by: osamahammad21 <[email protected]>
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

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

Signed-off-by: osamahammad21 <[email protected]>
Copy link
Contributor

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

: spc(spc_in), is_exact(is_exact_in), except_abutted(except_abutted_in)
{
}
bool operator<(const EdgeSpacingEntry& rhs) const { return spc < rhs.spc; }
Copy link
Member

Choose a reason for hiding this comment

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

Why only a partial ordering?

Copy link
Member Author

Choose a reason for hiding this comment

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

I only use this for getting the maximum spacing value. It doesn't matter if a < b or b < a if they both end up with the same spacing value.

@@ -932,6 +933,113 @@ bool Opendp::checkRegionOverlap(const Cell* cell,
// be fully contained by the cell's bounding box.
return result.empty();
}
namespace cell_edges {
Rect transformEdgeRect(const Rect& edge_rect,
Copy link
Member

Choose a reason for hiding this comment

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

Why can't you just apply the instance transform directly to the edge?

Copy link
Member Author

Choose a reason for hiding this comment

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

because x and y are the instance location (lower left corner point of the instance bounding box) not the origin point. applying the inst transform directly is not correct.

Copy link
Contributor

github-actions bot commented Feb 4, 2025

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

@maliberty maliberty merged commit abcc0ea into The-OpenROAD-Project:master Feb 4, 2025
11 checks passed
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.

3 participants