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 OR singleton from DRT core code #6658

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

bnmfw
Copy link
Contributor

@bnmfw bnmfw commented Feb 6, 2025

Fixes #6532.

DRT Core Dependencies on the ORD singleton

There are two main dependencies DRT has:

  1. Getting the Number of Threads.
  2. Making reads and writes to the ODB.

Number of Threads

Every case of the usage of the getThreadCount() was analyzed. In cases where the calls were close to the SWIG the value was plumbed through it. In other cases the value is stored in router_cgf_->MAX_THREADS and retrieved later.

For FlexDR.cpp calls, router_cfg_->MAXTHREADS always has the current thread count, so it can be used without harm.

Reads and Writes

Instead of running the code from the ORD singleton, the code on these calls was copied to where it is needed.
Write calls were simply substituted by:
db_->write(utl::StreamHandler(filename, true).getStream());
Which, although being a very convoluted, is compact and straight to the point.
The single read call is a bit extensive and copying it might not be the best solution. The copied code does not need interaction with the STA, as the original use did not actually enabled it usage.

bnmfw added 2 commits February 5, 2025 18:52
Signed-off-by: bernardo <[email protected]>
@bnmfw bnmfw added the drt Detailed Routing label Feb 6, 2025
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

Copy link
Contributor

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

@bnmfw bnmfw marked this pull request as ready for review February 13, 2025 21:34
@bnmfw bnmfw requested a review from maliberty February 13, 2025 21:34
@maliberty
Copy link
Member

@bnmfw I thought we merged a PR for this. Is this stale?

@bnmfw
Copy link
Contributor Author

bnmfw commented Feb 18, 2025

@maliberty I think you are referring to #6642. They both tackle the same issue (this closes it), the first solves coupling to the GUI and this solves coupling to the ORD singleton. If follows a similar decoupling pattern as the aforementioned PR, but it solves something different. I also wanted to make this a separate PR due to the debate that could arise from the Reads and Writes part.

@maliberty
Copy link
Member

I think the read/write_(lef/def/db) could all be moved to odb. The only tricky case seems to be readDb's callback to sta but that could be handled by an observer. If you make that change in a separate PR it will simplify this one.

Is just inconvenient to plumb the thread count to where it is needed? If so the router config avoid that though I would tend to just pass it through in a more explicit style.

@QuantamHD
Copy link
Collaborator

Amazing job with these changes @bnmfw. Your work is super impressive

Signed-off-by: bernardo <[email protected]>
@bnmfw
Copy link
Contributor Author

bnmfw commented Feb 22, 2025

@QuantamHD Thank you very much!

@bnmfw
Copy link
Contributor Author

bnmfw commented Feb 22, 2025

@maliberty This is ready for a new review. I have, however three points I'm not 100% on:

  1. The three uses of router_->getRouterConfiguration()->MAX_THREADS on RoutingCallBack.h. I'm not 100% sure if the thread count will be updated when the callbacks run as I'm not fully sure when they will run.
  2. I had the python problem we discussed briefly. One of the python test now needs to send the thread count as a parameter. I defaulted it to 1 for now, but it needs to be fixed to actually retrieve the thread count.
  3. The solution for the readDb does not seem to be the best one. The call actually does not need to interface with STA, as it does not sets "hierarchical" as true, so I could just transport the code to the DRT. I left the observer callbacks out, however, as it seem to not impact the regression tests I ran. I'd like to discuss any better solutions as I don't like very much how that part of the code turned out. I'm also running the ISPD and Secure CIs again to see if anything change, once they pass I will reopen the PR.

@bnmfw bnmfw marked this pull request as draft February 22, 2025 23:07
@maliberty maliberty marked this pull request as ready for review March 7, 2025 20:18
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.

DRT integration with GRT include issue
4 participants