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: decoupling of the Graphics classes and stable drt_lib #6550

Merged
merged 12 commits into from
Jan 28, 2025

Conversation

bnmfw
Copy link
Contributor

@bnmfw bnmfw commented Jan 18, 2025

Tackles #6532

This PR has a secure CI run associated with it and will be made open once it passes.

Abstract Classes

This PR makes so the core code of the DRT module is not dependent on the GUI. Abstract classes for the PA, TA and DR submodules were created so these are the classes the DRT module knows at compile time, while the concrete classes, the ones dependent on the GUI are passed during run time.

Graphic Classes as Class Attributes

The graphic classes used to be instantiated during the DRT execution and passed around. Now, all three are instantiated during initTritonRoute() and passed to the DRT. Then, they become class objects that are held by TritonRoute during it whole execution or until are moved to the respective submodule manager.

CMakeLists.txt modification

Now there is a clear separation between drt and drt_lib. I'm making the PR at a point where this separation can be done without any crashes, so it is stable. However it is still not possible to link drt_lib to the GRT without running into problems regarding the OR singleton dependency. This PR is becoming big, so I'm submitting it now, further linking problems will be tackled in a separate PR.

@bnmfw bnmfw requested review from maliberty and QuantamHD January 18, 2025 23:10
@bnmfw bnmfw added the drt Detailed Routing label Jan 18, 2025
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/src/dr/FlexDR_graphics.h Outdated Show resolved Hide resolved
bnmfw added 2 commits January 25, 2025 19:06
Signed-off-by: bernardo <[email protected]>
Signed-off-by: bernardo <[email protected]>
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/dr/FlexDR_graphics.h Show resolved Hide resolved
src/drt/src/pa/FlexPA_acc_point.cpp Show resolved Hide resolved
@bnmfw bnmfw marked this pull request as draft January 25, 2025 19:44
Signed-off-by: bernardo <[email protected]>
@bnmfw
Copy link
Contributor Author

bnmfw commented Jan 27, 2025

I'm not sure what clang-format is on about, I can't clang-format any file further

@bnmfw bnmfw changed the title drt: stable drt cmake list drt: decoupling of the Graphics classes and stable drt_lib Jan 27, 2025
@maliberty maliberty marked this pull request as ready for review January 27, 2025 18:47
@QuantamHD
Copy link
Collaborator

You need to make sure you're using the exact same clang format version. Ubuntu clang-format version 18.1.3

I've noticed there's subtle difference between local clang-format and the one installed in the CI lately.

Signed-off-by: Matt Liberty <[email protected]>
Comment on lines 231 to 232
std::unique_ptr<drt::FlexPA> pa_{nullptr};
std::unique_ptr<drt::FlexTA> ta_{nullptr};
Copy link
Member

Choose a reason for hiding this comment

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

Why do these need to be persistent now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't need now, but at least PA needs to be persistent to allow for PA with ODB callbacks, I was just starting to prepare things

Copy link
Member

Choose a reason for hiding this comment

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

I think incremental PA will capture a dirty list during the callbacks and then re-run in a single invocation for efficiency. We would still have an entry point in which to setup

Copy link
Member

Choose a reason for hiding this comment

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

What motivates the rearranging of this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tbh I moved this and the ppl file around when I was learning how to work with these kind of file. I was experimenting with the files and just tried to make them follow the same order.

@bnmfw bnmfw requested a review from maliberty January 28, 2025 18:54
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/dr/FlexDR_graphics.h Outdated Show resolved Hide resolved
src/drt/src/dr/FlexDR_graphics.h Outdated Show resolved Hide resolved
src/drt/src/dr/FlexDR_graphics.h Outdated Show resolved Hide resolved
src/drt/src/dr/FlexDR_graphics.h Outdated Show resolved Hide resolved
src/drt/src/dr/FlexDR_graphics.h Outdated Show resolved Hide resolved
src/drt/src/dr/FlexDR_graphics.h Outdated Show resolved Hide resolved
src/drt/src/pa/FlexPA_graphics.h Outdated Show resolved Hide resolved
src/drt/src/pa/FlexPA_graphics.h Outdated Show resolved Hide resolved
src/drt/src/ta/FlexTA_graphics.h Outdated Show resolved Hide resolved
src/drt/src/ta/FlexTA_graphics.h Outdated Show resolved Hide resolved
Signed-off-by: bernardo <[email protected]>
@maliberty maliberty enabled auto-merge January 28, 2025 21:45
@maliberty maliberty merged commit c4ec647 into The-OpenROAD-Project:master Jan 28, 2025
11 checks passed
@bnmfw bnmfw deleted the drt_linker branch January 28, 2025 22:35
openroad-ci pushed a commit to The-OpenROAD-Project-staging/OpenROAD that referenced this pull request Jan 29, 2025
…inker"

Fixes The-OpenROAD-Project#6607

This reverts commit c4ec647, reversing
changes made to 4a7c6dd.

Signed-off-by: Matt Liberty <[email protected]>
maliberty added a commit that referenced this pull request Jan 29, 2025
…vert

Revert "Merge pull request #6550 from bnmfw/drt_linker"
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