Set current design as associated data for Tcl commands #9176
Conversation
This is a fix for a crash that happens when calling initialize_floorplan from python. In a normal OpenROAD run, we associate the design with Tcl_SetAssocData in src/Main.cc but this function is not called if OpenROAD is being used as a library. Signed-off-by: Felipe Garay <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Signed-off-by: Felipe Garay <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes a crash when calling Tcl commands from Python by associating the current design with the Tcl interpreter. The change is correct and addresses the issue. I have one review comment with two suggestions for improvement: moving the association logic to the constructor for efficiency, and replacing the magic string "design" with a constant for better maintainability.
| auto openroad = getOpenRoad(); | ||
| ord::OpenRoad::setOpenRoad(openroad, /* reinit_ok */ true); | ||
| Tcl_Interp* tcl_interp = openroad->tclInterp(); | ||
| sta::Sta::setSta(openroad->getSta()); |
There was a problem hiding this comment.
While this correctly fixes the crash, I have a couple of suggestions for improvement on this line:
-
Efficiency:
evalTclStringwill now callTcl_SetAssocDataon every invocation. For improved efficiency, this association should be made only once in theDesignconstructor. This would involve removing this line and addingTcl_SetAssocData(getOpenRoad()->tclInterp(), "design", nullptr, this);inside theDesign::Designconstructor. -
Magic String: The string literal
"design"is a magic string. It would be best to define it as a constant in a shared header file and use the constant here and whereverTcl_GetAssocDatais called with this key. This improves maintainability and prevents errors from typos.
There was a problem hiding this comment.
I am ok with applying the efficiency suggestion but I don't think it is needed
There was a problem hiding this comment.
Is there any downside to changing it? If not it seems reasonable to do so (just to clarify that it is a one time binding).
There was a problem hiding this comment.
Yeah, I think I will apply it. I will be back in a moment.
There was a problem hiding this comment.
I would not do this. It makes it so the last design created is the only one you can evaltcl on. That's a bad experience
There was a problem hiding this comment.
Well, that explains the regressions that I was seeing on the tests.
Seems like we will need to call Tcl_SetAssocData every time we want to eval a tcl command. The performance impact is not clear to me but I bet it will be negligible compared to the execution of the command.
Wdyt @maliberty ?
3 participants
This is a fix for a crash that happens when calling initialize_floorplan from python. In a normal OpenROAD run, we associate the design with Tcl_SetAssocData in src/Main.cc but this function is not called if OpenROAD is being used as a library.