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

Integrate rsz repair design with gpl #6165

Merged

Conversation

openroad-robot
Copy link
Contributor

This PR introduces callback implementations for GPL, enabling us to retain the modifications made by RSZ during repair design iterations, including buffer insertion and resizing.

Currently, we perform six virtual repair design iterations in GPL. In these iterations, we execute changes but revert them at the end of each iteration. What remains in GPL are weights that prioritize cells with the worst slack. During the new non-virtual iteration, we keep all new buffers and resizing modifications made by repair design, allowing GPL to recognize these external changes.

A new TCL variable has been added to control when to switch from virtual to non-virtual iterations. Experiments indicate a slight preference for a sequence of three virtual iterations followed by three non-virtual iterations.

Timing-driven iterations are performed based on overflow values: [79, 64, 49, 29, 21, 15]. With the new TCL variable -keep_resize_below_overflow set to 0.3, we retain RSZ changes only when the overflow is below this threshold, resulting in three non-virtual iterations.

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: gudeh <augusto.berndt@precisioninno.com>
Signed-off-by: gudeh <augusto.berndt@precisioninno.com>
Signed-off-by: gudeh <augusto.berndt@precisioninno.com>
Signed-off-by: gudeh <augusto.berndt@precisioninno.com>
…ches

Signed-off-by: gudeh <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
now we need to skip both journalBegin() and journalRestore()

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
…erter for log values

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
…ility

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
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

There were too many comments to post at once. Showing the first 25 out of 51. Check the log or trigger a new build to see more.

@@ -248,6 +259,22 @@ bool GCell::isStdInstance() const
return !instance()->isMacro();
}

void GCell::print(utl::Logger* logger) const
{
if (insts_.size() > 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]

Suggested change
if (insts_.size() > 0)
if (!insts_.empty())
Additional context

/usr/include/c++/11/bits/stl_vector.h:1006: method 'vector'::empty() defined here

      empty() const _GLIBCXX_NOEXCEPT
      ^

@@ -248,6 +259,22 @@
return !instance()->isMacro();
}

void GCell::print(utl::Logger* logger) const
{
if (insts_.size() > 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: statement should be inside braces [google-readability-braces-around-statements]

Suggested change
if (insts_.size() > 0)
if (insts_.size() > 0) {

src/gpl/src/nesterovBase.cpp:265:

-   else
+   } else

Comment on lines 266 to 267
else
logger->report("print gcell insts_ empty! (filler cell)");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: statement should be inside braces [google-readability-braces-around-statements]

Suggested change
else
logger->report("print gcell insts_ empty! (filler cell)");
else {
logger->report("print gcell insts_ empty! (filler cell)");
}


void GPin::print(utl::Logger* log) const
{
if (pin()->dbITerm() != nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: statement should be inside braces [google-readability-braces-around-statements]

Suggested change
if (pin()->dbITerm() != nullptr)
if (pin()->dbITerm() != nullptr) {

src/gpl/src/nesterovBase.cpp:478:

-   else
+   } else

Comment on lines 479 to 480
else
log->report("pin()->dbIterm() is nullptr!");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: statement should be inside braces [google-readability-braces-around-statements]

Suggested change
else
log->report("pin()->dbIterm() is nullptr!");
else {
log->report("pin()->dbIterm() is nullptr!");
}

nextSLPDensityGrads_.push_back(FloatPoint());
nextSLPSumGrads_.push_back(FloatPoint());
prevSLPCoordi_.push_back(FloatPoint());
prevSLPWireLengthGrads_.push_back(FloatPoint());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use emplace_back instead of push_back [modernize-use-emplace]

Suggested change
prevSLPWireLengthGrads_.push_back(FloatPoint());
prevSLPWireLengthGrads_.emplace_back();

nextSLPSumGrads_.push_back(FloatPoint());
prevSLPCoordi_.push_back(FloatPoint());
prevSLPWireLengthGrads_.push_back(FloatPoint());
prevSLPDensityGrads_.push_back(FloatPoint());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use emplace_back instead of push_back [modernize-use-emplace]

Suggested change
prevSLPDensityGrads_.push_back(FloatPoint());
prevSLPDensityGrads_.emplace_back();

prevSLPCoordi_.push_back(FloatPoint());
prevSLPWireLengthGrads_.push_back(FloatPoint());
prevSLPDensityGrads_.push_back(FloatPoint());
prevSLPSumGrads_.push_back(FloatPoint());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use emplace_back instead of push_back [modernize-use-emplace]

Suggested change
prevSLPSumGrads_.push_back(FloatPoint());
prevSLPSumGrads_.emplace_back();

prevSLPWireLengthGrads_.push_back(FloatPoint());
prevSLPDensityGrads_.push_back(FloatPoint());
prevSLPSumGrads_.push_back(FloatPoint());
curCoordi_.push_back(FloatPoint());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use emplace_back instead of push_back [modernize-use-emplace]

Suggested change
curCoordi_.push_back(FloatPoint());
curCoordi_.emplace_back();

prevSLPDensityGrads_.push_back(FloatPoint());
prevSLPSumGrads_.push_back(FloatPoint());
curCoordi_.push_back(FloatPoint());
nextCoordi_.push_back(FloatPoint());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use emplace_back instead of push_back [modernize-use-emplace]

Suggested change
nextCoordi_.push_back(FloatPoint());
nextCoordi_.emplace_back();

@gudeh
Copy link
Contributor

gudeh commented Nov 15, 2024

I ran a secure-CI for this PR, on public designs I am only getting the following error on DRT.

asap7 jpeg (base)
  Last log file 5_2_route.log
  Found 1 errors in the logs.
      [ERROR CI-0000] No error message found. Here are the last 10 lines of the log.
  RECT -9 -14 9 14
          frVia: at ( 66933 33255 )
          VIA DEF:
          VIA VIA12 DEFAULT
            RECT -9 -11 9 11
            RECT -9 -9 9 9
            RECT -14 -9 14 9
          INSTTERM: (INST/CELL/TERM/NET) _084770_ OA21x2_ASAP7_75t_R A2 _012209_
          Command terminated by signal 11
          Elapsed time: 7:47.70[h:]min:sec. CPU time: user 11484.50 sys 225.34 (2503%). Peak memory: 13042140KB.

I see nothing obviously wrong on the net:
image

Other than this critical error I see only some metrics failing. I am waiting for results on private designs.

@maliberty
Copy link
Member

That error comes from drt Error: checkConnectivity break, net _012209_. Please talk with @osamahammad21 about it.

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
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

@@ -1168,4 +1262,14 @@ class GCellHandle
size_t index_;
};

inline bool isValidSigType(odb::dbSigType db_type)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: the parameter 'db_type' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]

Suggested change
inline bool isValidSigType(odb::dbSigType db_type)
inline bool isValidSigType(const odb::dbSigType& db_type)

inline bool isValidSigType(odb::dbSigType db_type)
{
if (db_type == odb::dbSigType::SIGNAL
|| db_type == odb::dbSigType::CLOCK) // || db_type ==
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: statement should be inside braces [google-readability-braces-around-statements]

Suggested change
|| db_type == odb::dbSigType::CLOCK) // || db_type ==
|| db_type == odb::dbSigType::CLOCK) { // || db_type ==

src/gpl/src/nesterovBase.h:1270:

-   else
+   } else

Comment on lines 1271 to 1272
else
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use 'else' after 'return' [readability-else-after-return]

Suggested change
else
return false;
return false;

|| db_type == odb::dbSigType::CLOCK) // || db_type ==
// odb::dbSigType::ANALOG)
return true;
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: statement should be inside braces [google-readability-braces-around-statements]

  else
      ^

this fix will not be applied because it overlaps with another fix

average_overflow_,
npVars_.keepResizeBelowOverflow);

if (!virtual_td_iter)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: statement should be inside braces [google-readability-braces-around-statements]

Suggested change
if (!virtual_td_iter)
if (!virtual_td_iter) {

src/gpl/src/nesterovPlace.cpp:457:

-       else
+       } else

virtual void inDbITermDestroy(odb::dbITerm*);

virtual void inDbNetCreate(odb::dbNet*) override;
virtual void inDbNetDestroy(odb::dbNet*) override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'virtual' is redundant since the function is already declared 'override' [modernize-use-override]

Suggested change
virtual void inDbNetDestroy(odb::dbNet*) override;
void inDbNetDestroy(odb::dbNet*) override;

virtual void inDbNetCreate(odb::dbNet*) override;
virtual void inDbNetDestroy(odb::dbNet*) override;

virtual void inDbInstSwapMasterAfter(odb::dbInst*);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'inDbInstSwapMasterAfter' overrides a member function but is not marked 'override' [clang-diagnostic-inconsistent-missing-override]

  virtual void inDbInstSwapMasterAfter(odb::dbInst*);
               ^
Additional context

src/odb/include/odb/dbBlockCallBackObj.h:82: overridden virtual function is here

  virtual void inDbInstSwapMasterAfter(dbInst*) {}
               ^

virtual void inDbNetCreate(odb::dbNet*) override;
virtual void inDbNetDestroy(odb::dbNet*) override;

virtual void inDbInstSwapMasterAfter(odb::dbInst*);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]

Suggested change
virtual void inDbInstSwapMasterAfter(odb::dbInst*);
void inDbInstSwapMasterAfter(odb::dbInst*) override;

virtual void inDbNetDestroy(odb::dbNet*) override;

virtual void inDbInstSwapMasterAfter(odb::dbInst*);
virtual void inDbPostMoveInst(odb::dbInst*) override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'virtual' is redundant since the function is already declared 'override' [modernize-use-override]

Suggested change
virtual void inDbPostMoveInst(odb::dbInst*) override;
void inDbPostMoveInst(odb::dbInst*) override;

@@ -201,6 +201,10 @@ class RouteBase
int inflationIterCnt() const;

void revertGCellSizeToMinRc();
void pushBackMinRcCellSize(int dx, int dy)
{
minRcCellSize_.push_back(std::make_pair(dx, dy));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use emplace_back instead of push_back [modernize-use-emplace]

Suggested change
minRcCellSize_.push_back(std::make_pair(dx, dy));
minRcCellSize_.emplace_back(dx, dy);

Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
Signed-off-by: Augusto Berndt <augusto.berndt@precisioninno.com>
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

Comment on lines 1269 to 1271
} else {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: do not use 'else' after 'return' [readability-else-after-return]

Suggested change
} else {
return false;
}
} return false;

Copy link
Contributor

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

@gudeh
Copy link
Contributor

gudeh commented Nov 19, 2024

The latest secure-CI run after a merge with master did not present the DRT issue previously happening.

Run ID 32 in this secure-CI is the one that presented the DRT issue, with asap7/jpeg.

@maliberty
Copy link
Member

Great we just need to wait for the private to finish

Comment on lines 1267 to 1270
if (db_type == odb::dbSigType::SIGNAL || db_type == odb::dbSigType::CLOCK) {
return true;
}
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (db_type == odb::dbSigType::SIGNAL || db_type == odb::dbSigType::CLOCK) {
return true;
}
return false;
return (db_type == odb::dbSigType::SIGNAL || db_type == odb::dbSigType::CLOCK);

Copy link
Contributor

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

@maliberty maliberty merged commit eb97b52 into The-OpenROAD-Project:master Nov 25, 2024
11 checks passed
@maliberty maliberty deleted the gpl-keep-rsz branch November 25, 2024 18:47
@maliberty
Copy link
Member

@QuantamHD FYI

@gudeh
Copy link
Contributor

gudeh commented Nov 25, 2024

Here there is some images showing placement density before the change (top) and after the change (bottom): https://drive.google.com/drive/folders/1vmZFFemVsjTfid3QStEZ5TrWDtVBwnlY?usp=drive_link

Here I did some experiments trying different non-virtual iterations (1, 2 and 3): https://drive.google.com/drive/folders/1U4u6opRNsjoS8MwOxLdjzQMA5LNFUdb2?usp=drive_link

@QuantamHD
Copy link
Collaborator

Beautiful!

@rovinski
Copy link
Collaborator

Indeed, it looks like it's working super well. Nice job!

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.

5 participants