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

rsz: use adequate buffers for each operation #5752

Merged
merged 13 commits into from
Oct 1, 2024

Conversation

AcKoucher
Copy link
Contributor

@AcKoucher AcKoucher commented Sep 16, 2024

For #5577,

Replaces #5693.

Changes:

  • Makes RSZ default behavior to not include clock buffers for its data sizing operations. There's one exception: As some technologies such as nangate45 don't have delay cells, until we have a better approach, it's better to consider clock buffers for hold violation repairing as these buffers' delay may be slighty higher and we'll need fewer insertions.
  • Keep the list of buffers generated by CTS for clock nets repairing function to use it.
  • Create new class in dbSta for distinguishing cell function type.

Note: On the previous PR the increase in the number of hold buffers inserted was leading gcd_resize rsz regression test to ERROR, with the changes here we have a subtle increase in the number of hold buffers inserted (99 --> 105), however it doesn't blow up.

Copy link
Contributor

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

src/rsz/src/Resizer.cc Outdated Show resolved Hide resolved
@AcKoucher AcKoucher marked this pull request as draft September 17, 2024 19:20
Copy link
Contributor

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

@AcKoucher AcKoucher changed the title rsz: use only data buffers except when doing hold repair rsz: use adequate buffers for each operation Sep 19, 2024
Copy link
Contributor

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

@AcKoucher
Copy link
Contributor Author

AcKoucher commented Sep 19, 2024

@maliberty Could you take a look if the approach here seems reasonable? I updated the description with the changes I added.

src/dbSta/include/db_sta/dbSta.hh Outdated Show resolved Hide resolved
src/rsz/src/Resizer.cc Outdated Show resolved Hide resolved
@AcKoucher
Copy link
Contributor Author

@maliberty @precisionmoon gdb shows that the failures in the Cpp tests happens, because some tests need to create a dbSta object. It blows up when it tries to create the sta::PatternMacth from within the instantiation of the new BufferUseAnalyser member of dbSta. However, it looks like the segfault comes from within the Tcl library (?):

#0  0x00007ffff7f12c12 in Tcl_GetThreadData () from /lib/x86_64-linux-gnu/libtcl8.6.so
#1  0x00007ffff7f04147 in ?? () from /lib/x86_64-linux-gnu/libtcl8.6.so
#2  0x00007ffff7f03e2f in Tcl_GetRegExpFromObj () from /lib/x86_64-linux-gnu/libtcl8.6.so
#3  0x000055555572c8ab in sta::PatternMatch::compileRegexp (this=0x555558607630) at /home/arthur/OpenROAD-flow-scripts/tools/OpenROAD/src/sta/util/PatternMatch.cc:84
#4  0x000055555572c6ab in sta::PatternMatch::PatternMatch (this=0x555558607630, pattern=0x55555748d55d ".*CLKBUF.*", is_regexp=true, nocase=true, interp=0x0)
    at /home/arthur/OpenROAD-flow-scripts/tools/OpenROAD/src/sta/util/PatternMatch.cc:36
#5  0x0000555555af2b94 in std::make_unique<sta::PatternMatch, char const (&) [11], bool, bool, decltype(nullptr)>(char const (&) [11], bool&&, bool&&, decltype(nullptr)&&) ()
    at /usr/include/c++/11/bits/unique_ptr.h:962
#6  0x0000555555af0284 in sta::BufferUseAnalyser::BufferUseAnalyser (this=0x555558607610) at /home/arthur/OpenROAD-flow-scripts/tools/OpenROAD/src/dbSta/src/dbSta.cc:883
#7  0x0000555555af0e45 in sta::dbSta::dbSta (this=0x555558607460) at /home/arthur/OpenROAD-flow-scripts/tools/OpenROAD/src/dbSta/src/../include/db_sta/dbSta.hh:104
#8  0x0000555555aec0ff in ord::makeDbSta () at /home/arthur/OpenROAD-flow-scripts/tools/OpenROAD/src/dbSta/src/dbSta.cc:78
#9  0x00005555555e1733 in rmp::AbcTest::SetUp (this=0x7ffff7c3e010) at /home/arthur/OpenROAD-flow-scripts/tools/OpenROAD/src/rmp/test/cpp/TestAbc.cc:63
#10 0x0000555555abd263 in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ()
#11 0x0000555555ab6511 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ()
#12 0x0000555555a925da in testing::Test::Run() ()
#13 0x0000555555a9316f in testing::TestInfo::Run() ()
#14 0x0000555555a93aad in testing::TestSuite::Run() ()
#15 0x0000555555aa3e5d in testing::internal::UnitTestImpl::RunAllTests() ()
#16 0x0000555555abe21c in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ()
#17 0x0000555555ab762b in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ()
#18 0x0000555555aa247f in testing::UnitTest::Run() ()
#19 0x0000555555ace189 in RUN_ALL_TESTS() ()
#20 0x0000555555ace102 in main ()

Is there something that need to be manually initialized before we can access the Tcl libraries from sta?

@maliberty
Copy link
Member

Is interp_ null?

@maliberty
Copy link
Member

The problem is you are initializing in the ctor but the call sequence is

    sta_ = std::unique_ptr<sta::dbSta>(ord::makeDbSta());
    sta_->initVars(Tcl_CreateInterp(), db_.get(), &logger_);

So initVars hasn't happened yet. I suggest dbSta contain by unique_ptr rather than by value and initialization happens in initVars.

@AcKoucher AcKoucher marked this pull request as ready for review September 27, 2024 14:18
@AcKoucher AcKoucher marked this pull request as draft September 27, 2024 14:21
Copy link
Contributor

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

Copy link
Contributor

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

@AcKoucher AcKoucher marked this pull request as ready for review September 27, 2024 20:15
@AcKoucher
Copy link
Contributor Author

AcKoucher commented Sep 27, 2024

@precisionmoon @maliberty After updating the FlowTests results and the last changes to avoid the rmp cpp tests segfaults all tests are passing.

I need to run a secure-ci and there'll probably be many metrics to update. I'll wait for #5790 to be merged before I run it.

@maliberty
Copy link
Member

@AcKoucher 5790 was merged

@AcKoucher
Copy link
Contributor Author

AcKoucher commented Sep 28, 2024

There are .ok files which haven't been properly updated on master. On src/gpl/test/simple01-td-tune.ok I see:

Differences found at line 63.
    - _276_ NOR2_X2 + PLACED ( 41514 9431 ) N ;
    - _276_ NOR2_X2 + PLACED ( 41516 9447 ) N ;

I'm fixing the ones regarding this PR.

@AcKoucher
Copy link
Contributor Author

Running CI

Copy link
Contributor

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

Signed-off-by: Arthur Koucher <[email protected]>
Copy link
Contributor

github-actions bot commented Oct 1, 2024

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

@maliberty maliberty merged commit 7d17168 into The-OpenROAD-Project:master Oct 1, 2024
10 checks passed
@AcKoucher AcKoucher deleted the rsz-clock-buffers branch October 1, 2024 22:07
@AcKoucher AcKoucher mentioned this pull request Oct 1, 2024
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants