drt: decoupling of the Graphics classes and stable drt_lib & graphic Factory #6642
Conversation
Signed-off-by: bernardo <[email protected]>
Signed-off-by: bernardo <[email protected]>
Signed-off-by: bernardo <[email protected]>
Signed-off-by: bernardo <[email protected]>
Signed-off-by: bernardo <[email protected]>
Signed-off-by: bernardo <[email protected]>
Signed-off-by: bernardo <[email protected]>
Signed-off-by: bernardo <[email protected]>
Signed-off-by: Matt Liberty <[email protected]> drt: clang-format Signed-off-by: bernardo <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
ready for review? |
Signed-off-by: bernardo <[email protected]>
|
@maliberty now it is |
| virtual AbstractDRGraphics* getDRGraphics() = 0; | ||
| virtual AbstractTAGraphics* getTAGraphics() = 0; | ||
| virtual AbstractPAGraphics* getPAGraphics() = 0; |
There was a problem hiding this comment.
Why does the factory have get methods? It is unusual for the pattern.
src/drt/src/TritonRoute.cpp
Outdated
| AbstractDRGraphics* graphics | ||
| = debug_->debugDR ? graphics_factory_->getDRGraphics() : nullptr; | ||
| worker->setGraphics(graphics); |
There was a problem hiding this comment.
Why not create and give ownership to the worker as before?
There was a problem hiding this comment.
the worker actually did not get the ownership of the pointer before. It just got a raw pointer and ownership was kept to the "scope of the function". I made the getXXGraphics to accommodate for this. Besides that I made every method for all three tools for the sake of keeping the pattern, even though I do not use all.
There was a problem hiding this comment.
Now the ownership is not with the "scope of the function" but factory class itself. What is the advantage of that? I don't expect a factory to own its products.
| // Define swig TCL commands. | ||
| auto tcl_interp = openroad->tclInterp(); | ||
| Drt_Init(tcl_interp); | ||
| sta::evalTclInit(tcl_interp, sta::drt_tcl_inits); |
There was a problem hiding this comment.
Did this need to be moved? Does the factory ctor depend on tcl somehow?
There was a problem hiding this comment.
This came before the factory, it was part of the old PR. tcl_interp being on TritonRoute.cpp creates linking issues. In most other tools on the project tcl_interp is always on the Make.cpp file. This is both for keeping this patterns and to allow for the drt_lib
| FlexTA ta(getDesign(), logger_, router_cfg_.get(), distributed_); | ||
| ta.setDebug(debug_.get(), db_); | ||
| ta.main(); | ||
| std::unique_ptr<FlexTA> ta = std::make_unique<FlexTA>( | ||
| getDesign(), logger_, router_cfg_.get(), distributed_); | ||
| if (debug_->debugTA) { | ||
| ta->setDebug(graphics_factory_->makeUniqueTAGraphics()); | ||
| } | ||
| ta->main(); |
There was a problem hiding this comment.
What motivates the change from stack to unique_ptr?
There was a problem hiding this comment.
Kinda arbitrary tbh. PA will have to be a unique_ptr created at initialization at some point for the GRT with PA to be possible. I planned to make this with DR, TA and PA, started it but reverted for this PR. This change is residual, I can revert it if you want, but at some point I think it will become a unique_ptr anyway.
bnmfw
changed the title
Signed-off-by: bernardo <[email protected]>
Tackles 6532.
This is PR #6550 but better.
The branch is the same and every new addition can be found starting from the "graphic factory" commit.
Graphics Factory
Instead of passing the three DR classes during the initialization of
TritonRoute, now a new class,GraphicsFactoryis responsible for providing both unique pointers and simple pointers for all the three classes. WheneverinitDesignis called,initGraphicswhich passes the necessary arguments for these classes is called, ensuring they are up to date.