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

Ant: new implementation using boost polygon #5098

Merged
merged 36 commits into from
Jun 5, 2024

Conversation

luis201420
Copy link
Contributor

@luis201420 luis201420 commented May 13, 2024

Fixes #3892
I ran a secure-ci with only the public designs, I'm waiting for the results

Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
…and repair_antenna

Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
@luis201420 luis201420 marked this pull request as draft May 13, 2024 23:19
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

There were too many comments to post at once. Showing the first 25 out of 32. Check the log or trigger a new build to see more.

src/ant/include/ant/AntennaChecker.hh Outdated Show resolved Hide resolved
src/ant/include/ant/AntennaChecker.hh Outdated Show resolved Hide resolved
src/ant/include/ant/AntennaChecker.hh Outdated Show resolved Hide resolved
src/ant/src/AntennaChecker.cc Outdated Show resolved Hide resolved
src/ant/src/AntennaChecker.cc Outdated Show resolved Hide resolved
src/ant/src/AntennaChecker.cc Outdated Show resolved Hide resolved
src/ant/src/AntennaChecker.cc Outdated Show resolved Hide resolved
src/ant/src/AntennaChecker.cc Outdated Show resolved Hide resolved
src/ant/src/AntennaChecker.cc Outdated Show resolved Hide resolved
src/ant/src/AntennaChecker.cc Outdated Show resolved Hide resolved
struct PinType;
struct GraphNode;

struct InfoType
Copy link
Collaborator

Choose a reason for hiding this comment

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

InfoType is too generic. What would you like to express here? The violation itself, the data computed for each gate?

}
};

typedef std::unordered_map<dbTechLayer*, InfoType> LayerInfoVector;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar question here. What is LayerInfo? Info is a bad name because it says nothing about what it represents in the code.


/////////////////////////////////////////////////////////////////////////////////
std::unordered_map<odb::dbTechLayer*, GraphNodeVector> node_by_layer_map_;
std::unordered_map<std::string, LayerInfoVector> info_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change info_ for a more meaningful name.

src/ant/include/ant/AntennaChecker.hh Outdated Show resolved Hide resolved
@@ -94,6 +153,9 @@ class AntennaChecker

void findMaxWireLength();

vector<Violation> getAntennaViolations2(dbNet* net,
Copy link
Collaborator

Choose a reason for hiding this comment

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

After finishing the reviews, I believe we will deprecate the old code, so we'll not have two functions with the same name.
@maliberty do you think we should completely remove the old code?

void calculateAreas();
void calculatePAR();
void calculateCAR();
int checkInfo(dbNet* db_net,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change checkInfo for a more meaningful name (related to the comments about InfoType being a bad struct name).

src/ant/src/AntennaChecker.cc Outdated Show resolved Hide resolved
@eder-matheus
Copy link
Collaborator

@luis201420 you will need to update the repair_antennas unit tests and the ant unit tests ok files.

Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
@luis201420
Copy link
Contributor Author

@luis201420 you will need to update the repair_antennas unit tests and the ant unit tests ok files.

I'm going to update it, I'm checking the differences in the unit tests.

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/ant/src/AntennaChecker.cc Outdated Show resolved Hide resolved
src/ant/src/AntennaChecker.cc Outdated Show resolved Hide resolved
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: luis201420 <tauro_luisito_20@hotmail.com>
Signed-off-by: Eder Monteiro <eder.matheus.monteiro@gmail.com>
Signed-off-by: Eder Monteiro <eder.matheus.monteiro@gmail.com>
Signed-off-by: Eder Monteiro <eder.matheus.monteiro@gmail.com>
Signed-off-by: Eder Monteiro <eder.matheus.monteiro@gmail.com>
Signed-off-by: Eder Monteiro <eder.matheus.monteiro@gmail.com>
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

namespace ant {

namespace gtl = boost::polygon;
using namespace gtl::operators;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use namespace using-directives; use using-declarations instead [google-build-using-namespace]

using namespace gtl::operators;
^

Signed-off-by: Eder Monteiro <eder.matheus.monteiro@gmail.com>
Signed-off-by: Eder Monteiro <eder.matheus.monteiro@gmail.com>
Signed-off-by: Eder Monteiro <eder.matheus.monteiro@gmail.com>
Signed-off-by: Eder Monteiro <eder.matheus.monteiro@gmail.com>
Signed-off-by: Eder Monteiro <eder.matheus.monteiro@gmail.com>
Signed-off-by: Eder Monteiro <eder.matheus.monteiro@gmail.com>
@eder-matheus eder-matheus marked this pull request as ready for review June 4, 2024 15:15
@eder-matheus
Copy link
Collaborator

Secure-ci is running with the latest updates. If everything passes, this PR should be ready to merge.

@eder-matheus
Copy link
Collaborator

@vvbandeira This is ready to merge.

@vvbandeira vvbandeira merged commit af4baf4 into The-OpenROAD-Project:master Jun 5, 2024
10 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.

Make antenna checker understand global route format
4 participants