rsz: Source `StaTclTypes.i` and clean up the binding #5277

povik · 2024-06-23T17:08:19Z

To get all the SWIG type conversion rules around basic STA types, it's best if we can source StaTclTypes.i. Here we make the change in the resizer binding, and use it to clean up some of the binding code. See the commits.

povik · 2024-06-23T17:10:46Z

The following STA patch is required, otherwise we get a conflict at linkage time. Please confirm the resizer change is desirable, I can open the OpenSTA PR for it.

diff --git a/tcl/StaTclTypes.i b/tcl/StaTclTypes.i
index 6127dea..abe3ca6 100644
--- a/tcl/StaTclTypes.i
+++ b/tcl/StaTclTypes.i
@@ -154,7 +154,7 @@ tclListNetworkSet(Tcl_Obj *const source,
     return nullptr;
 }
 
-StringSet *
+static StringSet *
 tclListSetConstChar(Tcl_Obj *const source,
 		    Tcl_Interp *interp)
 {
@@ -174,7 +174,7 @@ tclListSetConstChar(Tcl_Obj *const source,
     return nullptr;
 }
 
-StringSeq *
+static StringSeq *
 tclListSeqConstChar(Tcl_Obj *const source,
 		    Tcl_Interp *interp)
 {
@@ -194,7 +194,7 @@ tclListSeqConstChar(Tcl_Obj *const source,
     return nullptr;
 }
 
-StdStringSet *
+static StdStringSet *
 tclListSetStdString(Tcl_Obj *const source,
 		    Tcl_Interp *interp)
 {
@@ -279,7 +279,7 @@ setPtrTclList(SET_TYPE *set,
 
 ////////////////////////////////////////////////////////////////
 
-void
+static void
 tclArgError(Tcl_Interp *interp,
             const char *msg,
             const char *arg)
@@ -292,7 +292,7 @@ tclArgError(Tcl_Interp *interp,
   stringDelete(error);
 }
 
-void
+static void
 objectListNext(const char *list,
 	       const char *type,
 	       // Return values.

github-actions · 2024-06-23T17:11:52Z

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

povik · 2024-06-23T17:12:10Z

src/rsz/src/CMakeLists.txt

@@ -35,12 +35,22 @@

 include("openroad")

-swig_lib(NAME      rsz


Here I decided to split out the binding into a library of its own because we are polluting its include path a lot (see below in this file).

povik · 2024-06-23T17:16:27Z

As a result of the new include, and with the static functions STA patch, we get new warnings

[ 97%] Building CXX object src/rsz/src/CMakeFiles/rsz_swig.dir/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx.o
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:1926:1: warning: unused function 'tclListSetConstChar' [-Wunused-function]
tclListSetConstChar(Tcl_Obj *const source,
^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:1946:1: warning: unused function 'tclListSeqConstChar' [-Wunused-function]
tclListSeqConstChar(Tcl_Obj *const source,
^
...

If both openroad/opensta used C++17 we could silence with [[maybe_unused]] but I don't think that's the case.

github-actions · 2024-06-23T17:24:07Z

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

gadfort · 2024-06-23T17:40:32Z

@povik is it possible to use %import instead of %include to avoid the need for the sta patch?

povik · 2024-06-23T18:13:17Z

is it possible to use %import instead of %include to avoid the need for the sta patch?

@gadfort changing that and reverting the sta patch I get the failures below. I am not familiar with SWIG so it's hard for me to say how much work it would take to make %import work.

/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:3096:12: error: use of undeclared identifier 'tclListSeqPtr'
    arg2 = tclListSeqPtr<const Instance*>(objv[2], SWIGTYPE_p_Instance, interp);
           ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:3126:12: error: use of undeclared identifier 'tclListSeqPtr'
    arg1 = tclListSeqPtr<const Instance*>(objv[1], SWIGTYPE_p_Instance, interp);
           ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:3582:5: error: use of undeclared identifier 'seqPtrTclList'
    seqPtrTclList<NetSeq, Net>(nets, SWIGTYPE_p_Net, interp);
    ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:3613:5: error: use of undeclared identifier 'setPtrTclList'
    setPtrTclList<PinSet, Pin>(pins, SWIGTYPE_p_Pin, interp);
    ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:4371:5: error: use of undeclared identifier 'seqPtrTclList'
    seqPtrTclList<NetSeq, Net>(result, SWIGTYPE_p_Net, interp);
    ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:4657:24: error: use of undeclared identifier 'cmdNetwork'
    Network *network = cmdNetwork();
                       ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:4658:12: error: use of undeclared identifier 'tclListNetworkSet'
    arg1 = tclListNetworkSet<PinSet, Pin>(objv[1], SWIGTYPE_p_Pin, interp, network);
           ^
/Users/pk/repos/openroad/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:4676:5: error: use of undeclared identifier 'setTclList'
    setTclList<PinSet, Pin>(result, SWIGTYPE_p_Pin, interp);
    ^

gadfort · 2024-06-23T19:44:56Z

@povik thanks for checking, I was hoping that would have solved it.

maliberty · 2024-06-24T10:20:02Z

I appears %import doesn't bring the template functions into scope and %include leads to duplication. Using static works at the minor cost of having duplicate instantiations in the program.

A slightly nicer patch would be to pull these helper functions out of the .i into a regular c++ where they could more easily be shared. I believe then %import could be made to work. I'm not sure its worth the bother though.

povik · 2024-06-24T10:40:27Z

@maliberty Should I open a PR against OpenROAD's or upstream STA?

maliberty · 2024-06-24T10:42:06Z

@maliberty Should I open a PR against OpenROAD's or upstream STA?

upstream (which requires a CLA)

povik · 2024-07-31T17:56:40Z

parallaxsw/OpenSTA#62, which was blocking this PR, has now been merged

Signed-off-by: Martin Povišer <[email protected]>

Now that we source `StaTclTypes.i` we can rely on it to supply much of the type conversion rules. Only keep those which are unique to the resizer, and for those try to use the generic conversion helpers (e.g. `setPtrTclList`). Signed-off-by: Martin Povišer <[email protected]>

Signed-off-by: Martin Povišer <[email protected]>

povik · 2024-09-03T20:17:04Z

Rebased and inserted sign-offs, this could be good to go now

github-actions · 2024-09-03T20:19:20Z

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

povik · 2024-09-03T20:43:38Z

The CI failures are due to some of the helpers included from StaTclTypes.i ending up not getting used in Resizer.i (but marked static):

/tmp/workspace/OpenROAD-Public_PR-5277-merge/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:2132:1: error: 'void sta::objectListNext(const char*, const char*, bool&, const char*&)' defined but not used [-Werror=unused-function]
 2132 | objectListNext(const char *list,
      | ^~~~~~~~~~~~~~
/tmp/workspace/OpenROAD-Public_PR-5277-merge/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:2119:1: error: 'void sta::tclArgError(Tcl_Interp*, const char*, const char*)' defined but not used [-Werror=unused-function]
 2119 | tclArgError(Tcl_Interp *interp,
      | ^~~~~~~~~~~
/tmp/workspace/OpenROAD-Public_PR-5277-merge/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:2012:1: error: 'sta::StringSeq* sta::tclListSeqConstChar(Tcl_Obj*, Tcl_Interp*)' defined but not used [-Werror=unused-function]
 2012 | tclListSeqConstChar(Tcl_Obj *const source,
      | ^~~~~~~~~~~~~~~~~~~
/tmp/workspace/OpenROAD-Public_PR-5277-merge/build/src/rsz/src/CMakeFiles/rsz_swig.dir/ResizerTCL_wrap.cxx:1992:1: error: 'sta::StringSet* sta::tclListSetConstChar(Tcl_Obj*, Tcl_Interp*)' defined but not used [-Werror=unused-function]
 1992 | tclListSetConstChar(Tcl_Obj *const source,
      | ^~~~~~~~~~~~~~~~~~~

Should I open another STA patch to have [[maybe_unused]] on those helpers? That sounds the cleanest, though I am not sure OpenSTA is C++17.

maliberty · 2024-09-10T20:35:15Z

Looks like it uses c++17 https://github.com/parallaxsw/OpenSTA/blob/bd13bc409ebac682aa25778f770458829ff14cc2/CMakeLists.txt#L565 so you can try a PR there

povik commented Jun 23, 2024

View reviewed changes

povik mentioned this pull request Jun 23, 2024

Phase 1 for adding buffer removal to setup fixing #5100

Merged

povik mentioned this pull request Jul 30, 2024

Add static qualifier to StaTclTypes.i helpers parallaxsw/OpenSTA#62

Merged

povik added 4 commits September 3, 2024 21:38

rsz: Remove duplicate include

7a0124e

Signed-off-by: Martin Povišer <[email protected]>

rsz: Source StaTclTypes.i in binding code

3165af0

Signed-off-by: Martin Povišer <[email protected]>

rsz: Remove obsolete declarations

2646131

Signed-off-by: Martin Povišer <[email protected]>

povik force-pushed the rsz-swig-cleanup branch from 7ef4685 to 2646131 Compare September 3, 2024 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rsz: Source `StaTclTypes.i` and clean up the binding #5277

rsz: Source `StaTclTypes.i` and clean up the binding #5277

povik commented Jun 23, 2024

povik commented Jun 23, 2024 •

edited

Loading

github-actions bot commented Jun 23, 2024

povik Jun 23, 2024 •

edited

Loading

povik commented Jun 23, 2024

github-actions bot commented Jun 23, 2024

gadfort commented Jun 23, 2024

povik commented Jun 23, 2024

gadfort commented Jun 23, 2024

maliberty commented Jun 24, 2024

povik commented Jun 24, 2024

maliberty commented Jun 24, 2024

povik commented Jul 31, 2024

povik commented Sep 3, 2024

github-actions bot commented Sep 3, 2024

povik commented Sep 3, 2024

maliberty commented Sep 10, 2024

rsz: Source StaTclTypes.i and clean up the binding #5277

Are you sure you want to change the base?

rsz: Source StaTclTypes.i and clean up the binding #5277

Conversation

povik commented Jun 23, 2024

povik commented Jun 23, 2024 • edited Loading

github-actions bot commented Jun 23, 2024

povik Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

povik commented Jun 23, 2024

github-actions bot commented Jun 23, 2024

gadfort commented Jun 23, 2024

povik commented Jun 23, 2024

gadfort commented Jun 23, 2024

maliberty commented Jun 24, 2024

povik commented Jun 24, 2024

maliberty commented Jun 24, 2024

povik commented Jul 31, 2024

povik commented Sep 3, 2024

github-actions bot commented Sep 3, 2024

povik commented Sep 3, 2024

maliberty commented Sep 10, 2024

rsz: Source `StaTclTypes.i` and clean up the binding #5277

rsz: Source `StaTclTypes.i` and clean up the binding #5277

povik commented Jun 23, 2024 •

edited

Loading

povik Jun 23, 2024 •

edited

Loading