Skip to content

Conversation

@rafaelmoresco
Copy link
Contributor

Closes #9238

Current DPL and DPO ignore the allowed symmetries indicated in the LEF file.
Added symmetry checks for every movement attempt in DPL and DPO.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces LEF symmetry checks in DPL and DPO, which is a valuable addition. The implementation is mostly correct, but there are several instances of code duplication that should be addressed to improve maintainability. I've provided suggestions to refactor the duplicated logic into helper functions and to simplify some of the new checks. Addressing these points will make the code cleaner and more robust.

Comment on lines +166 to +176
auto* dbMaster = ndi->getDbInst()->getMaster();
unsigned masterSym = 0;
if (dbMaster->getSymmetryX()) {
masterSym |= Symmetry_X;
}
if (dbMaster->getSymmetryY()) {
masterSym |= Symmetry_Y;
}
if (dbMaster->getSymmetryR90()) {
masterSym |= Symmetry_ROT90;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This logic to calculate masterSym is duplicated in Place.cpp, detailed_manager.cxx, and twice in this file. To improve maintainability and reduce code duplication, you should extract this into a new static helper function within the DetailedOrient class.

For example, you could add the following to detailed_orient.h:

static unsigned getMasterSymmetry(odb::dbMaster* master);

And implement it in detailed_orient.cxx:

unsigned DetailedOrient::getMasterSymmetry(odb::dbMaster* master)
{
  unsigned masterSym = 0;
  if (master->getSymmetryX()) {
    masterSym |= Symmetry_X;
  }
  if (master->getSymmetryY()) {
    masterSym |= Symmetry_Y;
  }
  if (master->getSymmetryR90()) {
    masterSym |= Symmetry_ROT90;
  }
  return masterSym;
}

Then, you can replace this block and other occurrences with a call to this new helper function. For example:

  • Here and in orientSingleHeightCellForRow: unsigned masterSym = getMasterSymmetry(ndi->getDbInst()->getMaster());
  • In detailed_manager.cxx: unsigned masterSym = DetailedOrient::getMasterSymmetry(nd->getDbInst()->getMaster());
  • In Place.cpp: unsigned masterSym = dpl::DetailedOrient::getMasterSymmetry(cell->getDbInst()->getMaster()); (you'll need to include optimization/detailed_orient.h).
  auto* dbMaster = ndi->getDbInst()->getMaster();
  unsigned masterSym = getMasterSymmetry(dbMaster);

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

@rafaelmoresco
Copy link
Contributor Author

@maliberty is there any problem in using the DPO symmetry checker in DPL like gemini suggested?

@rafaelmoresco rafaelmoresco marked this pull request as draft January 16, 2026 20:28
@rafaelmoresco rafaelmoresco marked this pull request as ready for review January 16, 2026 20:52
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: rafaelmoresco <[email protected]>
@github-actions
Copy link
Contributor

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

@maliberty
Copy link
Member

Ready for review?

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.

Openroad ignores the SYMMETRY parameter from the LEF file for a standard cell

2 participants