Conversation
Signed-off-by: bnmfw <bernardoborgessandoval@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the initial draft for the EEQ (Equivalent LEF) feature, which aims to address M1 track misalignment by swapping cell masters. The changes include new methods for checking M1 misalignment, swapping instances, and fixing parity. While the author notes this is a draft, the core functionality is present. Future formalization should focus on improving code clarity, removing commented-out code, and addressing magic numbers.
| // Save displacement stats before updating instance DB locations. | ||
| findDisplacementStats(); | ||
| updateDbInstLocations(); | ||
| // fixParity(); |
There was a problem hiding this comment.
This line is commented out. Please either remove commented-out code or provide a clear explanation for why it is temporarily disabled.
| auto inst = cell->getDbInst(); | ||
| std::string master_name = inst->getMaster()->getName(); | ||
| bool isEEQ = master_name.size() > 4 && master_name.substr(master_name.size() - 4) == "_EEQ"; | ||
| int site_witdh = 480; |
There was a problem hiding this comment.
The value 480 appears to be a magic number. It would improve maintainability and readability to define this as a named constant with a clear explanation of its purpose.
| int site_witdh = 480; | |
| const int site_width = 480; |
References
- Define tunable parameters as named constants instead of using hardcoded magic numbers.
|
|
||
| const bool alignment_swapped = (inst->getOrient() == dbOrientType::R180 || inst->getOrient() == dbOrientType::MY); | ||
| int width = inst->getMaster()->getWidth()/site_witdh; | ||
| inst->getITerms(); |
There was a problem hiding this comment.
The return value of inst->getITerms() is not used, and this call does not appear to have any side effects relevant to the function's logic. This line can be removed to improve code clarity and efficiency.
|
|
||
| bool swap = (site % 2) == 0; | ||
| if (bad_mirror) { | ||
| swap = !swap; | ||
| } | ||
| if (isEEQ) { | ||
| swap = !swap; | ||
| } | ||
| return swap; |
There was a problem hiding this comment.
The boolean logic for determining swap can be simplified using XOR operations, which can make the intent clearer and more concise.
bool swap = (site % 2 == 0) ^ bad_mirror ^ isEEQ;
| bool swap = (site % 2) == 0; | |
| if (bad_mirror) { | |
| swap = !swap; | |
| } | |
| if (isEEQ) { | |
| swap = !swap; | |
| } | |
| return swap; | |
| bool swap = (site % 2 == 0) ^ bad_mirror ^ isEEQ; |
| if (cell->getType() != Node::CELL) { | ||
| continue; | ||
| } | ||
| // inst->get |
There was a problem hiding this comment.
This line is commented out. Please either remove commented-out code or provide a clear explanation for why it is temporarily disabled.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| bool isEEQ = master_name.size() > 4 && master_name.substr(master_name.size() - 4) == "_EEQ"; | ||
| int site_witdh = 480; | ||
|
|
||
| const bool alignment_swapped = (inst->getOrient() == dbOrientType::R180 || inst->getOrient() == dbOrientType::MY); |
There was a problem hiding this comment.
warning: use of undeclared identifier 'dbOrientType'; did you mean 'odb::dbOrientType'? [clang-diagnostic-error]
| const bool alignment_swapped = (inst->getOrient() == dbOrientType::R180 || inst->getOrient() == dbOrientType::MY); | |
| const bool alignment_swapped = (inst->getOrient() == dbOrientType::R180 || inst->getOrient() == odb::dbOrientType::MY); |
Additional context
src/dpl/src/PlacementDRC.h:11: 'odb::dbOrientType' declared here
class dbOrientType;
^| bool isEEQ = master_name.size() > 4 && master_name.substr(master_name.size() - 4) == "_EEQ"; | ||
| int site_witdh = 480; | ||
|
|
||
| const bool alignment_swapped = (inst->getOrient() == dbOrientType::R180 || inst->getOrient() == dbOrientType::MY); |
There was a problem hiding this comment.
warning: use of undeclared identifier 'dbOrientType'; did you mean 'odb::dbOrientType'? [clang-diagnostic-error]
| const bool alignment_swapped = (inst->getOrient() == dbOrientType::R180 || inst->getOrient() == dbOrientType::MY); | |
| const bool alignment_swapped = (inst->getOrient() == odb::dbOrientType::R180 || inst->getOrient() == dbOrientType::MY); |
Additional context
src/dpl/src/PlacementDRC.h:11: 'odb::dbOrientType' declared here
class dbOrientType;
^
| queryBox.max_corner().y()); | ||
| } | ||
|
|
||
| bool Opendp::M1Missaligned(Node* cell) |
There was a problem hiding this comment.
Better naming: isM1Missaligned()
| place(); | ||
| fixParity(); |
There was a problem hiding this comment.
I wonder if this changes lead to failures, for example the ones checked with checkDRC(). You can check with check_placement TCL command.
If they do happen, you could change the call of your function to happen together with mapMove() and shiftMove() since they already provide means of checking for failures during the move of the cells.
2 participants
Ignore this for now, this is the most draftiests draft I have ever made, I'm opening this in this state only to have the PR to link to.
The EEQ Feature
Some PDKs use the LEF58 EEQ Feature. Essentially multiple equivalent LEFs of the same cells in the same sizing are provided. Each one has a different alignment with the tracks of metal:
This essentially ensures that no matter where the instance is placed, if there are default you have a LEF that has all pins aligned in tracks, so every access is guaranteed to be on grid.
This Patch
This PR contains a VERY simple solution for the EEQ problem used for a specific advanced PDK. In this case misalignment can only happen at the M1 tracks and it is possible to determine the correct version only by name. Please do not pay mind to the name of the functions.
I intend on formalizing this PR later to bring it up to standard. Currently the solution is janky.