The-OpenROAD-Project / OpenROAD

OpenROAD's unified application implementing an RTL-to-GDS Flow. Documentation at https://openroad.readthedocs.io/en/latest/
https://theopenroadproject.org/
BSD 3-Clause "New" or "Revised" License
1.62k stars 561 forks source link

drt: getUpdatedXform(false) elimination #6141

Open bnmfw opened 1 week ago

bnmfw commented 1 week ago

This PR is ISPD and Secure CI Safe.

The problem

This PR solves part of the conflicts presented in issue #5712. In io::Parser::setInst() the Transform object from odb::dbInst is copied and held as a private attribute of drt::frInst. This new Transform is also modified to have its origin at the lower left corner, which is not always accurate. Furthermore, an auxiliary function, getUpdatedXform, exists to undo this origin transformation, trying to get back to the original odb Transform. Unfortunately, even this strategy is not error proof, as this patch to get back to the odb inst in done on the private drt::frInst which might become obsolete if the instance is moved, as the update in the Transform to get the new cell location is not precise.

Solutions

The elimination of getUpdatedXform() cases allowed for the elimination of updateXform() and simplifications of functions that used it to simply use getDBTransform().

Furthermore, getUpdatedXform() was refactored to eliminate the now unused functionality. This reduced the function to one that simply gets the Transformation and sets the rotation as R0. getUpdatedXform() was renamed to getNoRotationTransform() accordingly (this is temporary, the objective is to eliminate the function altogether).

In a lot of cases I noticed that the code manually got the Transformation and set its rotation to R0. All this cases were substituted by getNoRotationTransform(). This might help eliminating the drt::Inst Transform, as it reduces many uses to a single function.

frInstTerm::getShapes and frInstTerm::getBBox() had dead code eliminated. Both functions had a mode in which they used the drt::Inst Transform, and a mode where they used the old getUpdatedXform() but only used the latter. They were refactored to only have the second case and no alternative mode.

Finally some redundant code was eliminated regarding Transform manipulation. When a transform is created it always defaults to having the origin at 0,0 and rotation as R0, specifying that upon creation of the Transform is redundant. Also, ideally there wont even be a way to set this parameters in the future, as odb will handle the Transform, so the less the drt sets this parameters the better. Fixture::makeInst was simplified to avoid these redundancies.

gc_Test Patch

Now that the creation of an inst requires the association to a odb::Inst some of the tests on gc_test had to be patched to accommodate this initialization. Fixture::createDummyInst was created to accommodate for this requirement. The only required part of the instance is the transform which is initialized,

github-actions[bot] commented 1 week ago

clang-tidy review says "All clean, LGTM! :+1:"

github-actions[bot] commented 1 week ago

clang-tidy review says "All clean, LGTM! :+1:"

github-actions[bot] commented 1 week ago

clang-tidy review says "All clean, LGTM! :+1:"

github-actions[bot] commented 1 day ago

clang-tidy review says "All clean, LGTM! :+1:"

github-actions[bot] commented 23 hours ago

clang-tidy review says "All clean, LGTM! :+1:"