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

DPO: support edge spacing #6813

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

osamahammad21
Copy link
Member

  • Add Journal tracking for easier redo/undo operations.
  • Move Grid class from dpl to dpo as an initial step to merge both modules.
  • Fix wrong interpretation of row site count limit.
  • Add support to LEF58_EDGESPACINGTABLE rule

Signed-off-by: osamahammad21 <osama21@aucegypt.edu>
@osamahammad21 osamahammad21 requested a review from maliberty March 4, 2025 17:11
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

// GridY gridEndY(const Cell* cell) const;

int gridYToDbu(int y) const;
int gridXToDbu(int y) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function 'dpo::Grid::gridXToDbu' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]

  int gridXToDbu(int y) const;
      ^
Additional context

src/dpo/src/Grid.cxx:164: the definition seen here

int Grid::gridXToDbu(int x) const
          ^

src/dpo/src/Grid.h:107: differing parameters are named here: ('y'), in definition: ('x')

  int gridXToDbu(int y) const;
      ^

auto node = actions[i].getNode();
updateBins(actions[i].getNode(),
actions[i].getOrigLeft() + 0.5 * node->getWidth(),
actions[i].getOrigBottom() + 0.5 * node->getHeight(),
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use range-based for loop instead [modernize-loop-convert]

Suggested change
actions[i].getOrigBottom() + 0.5 * node->getHeight(),
for (const auto & action : actions) {
auto node = action.getNode();
updateBins(action.getNode(),
action.getOrigLeft() + 0.5 * node->getWidth(),
action.getOrigBottom() + 0.5 * node->getHeight(),

auto node = actions[i].getNode();
updateBins(actions[i].getNode(),
actions[i].getNewLeft() + 0.5 * node->getWidth(),
actions[i].getNewBottom() + 0.5 * node->getHeight(),
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use range-based for loop instead [modernize-loop-convert]

Suggested change
actions[i].getNewBottom() + 0.5 * node->getHeight(),
for (const auto & action : actions) {
auto node = action.getNode();
updateBins(action.getNode(),
action.getNewLeft() + 0.5 * node->getWidth(),
action.getNewBottom() + 0.5 * node->getHeight(),

orientPtr_->orientAdjust(nodes[i], newOri[i]);
}
for (int i = 0; i < changes.size(); i++) {
mgrPtr_->redo(changes[i], true);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use range-based for loop instead [modernize-loop-convert]

Suggested change
mgrPtr_->redo(changes[i], true);
for (const auto & change : changes) {
mgrPtr_->redo(change, true);

const std::vector<int>& newLeft,
const std::vector<int>& newBottom,
const std::vector<unsigned>& newOri) override;
double delta(const Journal& journal);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'delta' overrides a member function but is not marked 'override' [clang-diagnostic-inconsistent-missing-override]

  double delta(const Journal& journal);
         ^
Additional context

src/dpo/src/detailed_objective.h:65: overridden virtual function is here

  virtual double delta(const Journal& journal) = 0;
                 ^

const std::vector<int>& newLeft,
const std::vector<int>& newBottom,
const std::vector<unsigned>& newOri) override;
double delta(const Journal& journal);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: annotate this function with 'override' or (rarely) 'final' [modernize-use-override]

Suggested change
double delta(const Journal& journal);
double delta(const Journal& journal) override;

orientPtr_->orientAdjust(nodes[i], newOri[i]);
}
for (int i = 0; i < changes.size(); i++) {
mgrPtr_->redo(changes[i], true);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use range-based for loop instead [modernize-loop-convert]

Suggested change
mgrPtr_->redo(changes[i], true);
for (const auto & change : changes) {
mgrPtr_->redo(change, true);

class Master
{
public:
Master() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use '= default' to define a trivial default constructor [modernize-use-equals-default]

Suggested change
Master() {}
Master() = default;

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.

1 participant