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

drt: getUpdatedXform(false) elimination #6141

Merged

Conversation

bnmfw
Copy link
Contributor

@bnmfw bnmfw commented Nov 11, 2024

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

  • New, temporary functions, called getDBTransform and setDBInst were created. Whenever getUpdatedXform() is called, getDBTransform is used as that is what the original function is trying to do. Once the private drt::frInst Transform is eliminted getDBTransform will be renamed to become simply getTransform. getUpdatedXform(true) use cases are not eliminated in this PR.
  • DesignCallBack::inDbPreMoveInst was included to correctly "move" the instance in the R Tree strucute.

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,

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/drt/src/db/obj/frInstTerm.cpp Outdated Show resolved Hide resolved
src/drt/src/db/obj/frInstTerm.cpp Outdated Show resolved Hide resolved
@bnmfw bnmfw force-pushed the drt_dbTransform_refactor branch from f68ddee to 2200928 Compare November 11, 2024 23:21
Copy link
Contributor

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

@bnmfw bnmfw added the drt Detailed Routing label Nov 12, 2024
@bnmfw bnmfw requested a review from osamahammad21 November 12, 2024 20:15
Copy link
Contributor

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

@bnmfw bnmfw force-pushed the drt_dbTransform_refactor branch from 1e9a755 to c87a7dd Compare November 13, 2024 20:06
@bnmfw bnmfw requested a review from maliberty November 13, 2024 20:25
Copy link
Contributor

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

src/drt/src/db/obj/frInst.h Outdated Show resolved Hide resolved
src/drt/src/db/obj/frInst.h Outdated Show resolved Hide resolved
src/drt/test/fixture.cpp Outdated Show resolved Hide resolved
@@ -94,6 +95,15 @@ class frInst : public frRef
void setOrigin(const Point& tmpPoint) override { xform_.setOffset(tmpPoint); }
dbTransform getTransform() const override { return xform_; }
Copy link
Member

Choose a reason for hiding this comment

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

Is this used anywhere now?

Copy link
Contributor Author

@bnmfw bnmfw Nov 13, 2024

Choose a reason for hiding this comment

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

Unfortunatly, yes. getTransform() itself is used in the now renamed getNoRotationTransform() and in some other cases. Both setOrigin() and setOrient() still see some uses. I intend to completely eliminate these two functions, but I'm not sure it is possible. (I'm not 100% sure about what I'm about to say) During Pin Access virtual vias have to be created to check for DRVs when considering if a Access Point is valid. These vias exists only on the drt, and don't have an odb counterpart so during checking they need to use and configure the drt Transform.

Copy link
Member

Choose a reason for hiding this comment

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

Does it need to be public?

@bnmfw bnmfw force-pushed the drt_dbTransform_refactor branch 2 times, most recently from bc8bf0f to 1bd92bf Compare November 21, 2024 23:51
Signed-off-by: bernardo <[email protected]>
Copy link
Contributor

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

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/drt/test/fixture.cpp Outdated Show resolved Hide resolved
src/drt/test/fixture.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

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

@bnmfw bnmfw marked this pull request as ready for review November 22, 2024 06:18
@bnmfw bnmfw requested a review from osamahammad21 November 22, 2024 17:59
src/drt/test/fixture.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

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

@osamahammad21 osamahammad21 merged commit 7e8f05f into The-OpenROAD-Project:master Nov 28, 2024
11 checks passed
@bnmfw bnmfw removed the In Progress label Nov 28, 2024
@bnmfw bnmfw deleted the drt_dbTransform_refactor branch November 28, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
drt Detailed Routing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants