gpl: revert if diverge #6561

openroad-ci · 2025-01-20T21:24:37Z

This PR introduces a new TCL command that allows GPL to finish with an incomplete solution if divergence is detected. The solution is considered incomplete because it does not meet the 0.10 target for total overflow.

With the new option enabled gpl will snapshot the placement state (using a similar methodology as routability-mode already does), the snapshot is saved considering the smallest HPWL and when the total overflow is below 0.25. In the event of divergence, GPL will revert to the snapshot and complete the process with this solution instead of failing and throwing an error.

This approach has been tested successfully on Megaboom and nangate45/bp_be designs in cases of divergence.

Signed-off-by: Augusto Berndt <[email protected]>

adjust some routability messages Signed-off-by: Augusto Berndt <[email protected]>

Signed-off-by: Augusto Berndt <[email protected]>

github-actions

clang-tidy made some suggestions

github-actions · 2025-01-20T21:28:50Z

src/gpl/src/nesterovPlace.cpp

+                   diverge_snapshot_iter_,
+                   diverge_snapshot_average_overflow_unscaled_,
+                   min_hpwl_);
+        curA = diverge_snapshotA;


warning: Value stored to 'curA' is never read [clang-analyzer-deadcode.DeadStores]

curA = diverge_snapshotA; ^

Additional context

src/gpl/src/nesterovPlace.cpp:572: Value stored to 'curA' is never read

curA = diverge_snapshotA; ^

Signed-off-by: Augusto Berndt <[email protected]>

src/gpl/src/replace.tcl

oharboe · 2025-01-21T06:56:48Z

@gudeh Is this a solution or a way to cope with the algorithm diverging unexpectedly/undesirably?

gudeh · 2025-01-21T13:43:42Z

@gudeh Is this a solution or a way to cope with the algorithm diverging unexpectedly/undesirably?

Hi @oharboe, this is just a way to take some pressure away from this problem, a way to cope with the divergence. The actual solution for the divergence occurring in gpl is still under investigation.

gudeh · 2025-01-21T13:46:00Z

This PR should help avoiding the errors reported at issues #6542 and #6465

gudeh · 2025-01-22T12:55:08Z

This PR only adds the new non-default option, it has no change in functionality. In another ORFS PR I activated the new functionality only for nangate45/BP_BE. We have no metrics changing and both PR could be merged separately

oharboe · 2025-01-22T13:10:56Z

@gudeh FYI The-OpenROAD-Project/OpenROAD-flow-scripts#2705

eder-matheus · 2025-01-23T16:44:26Z

src/gpl/src/nesterovPlace.cpp

+        // log_->report("[NesterovSolve] Snapshot for divergence saved at iter =
+        // {}", iter);


Remove commented code.

eder-matheus · 2025-01-23T16:46:40Z

src/gpl/src/replace.tcl

+    utl::warn "GPL" 153 \
+      "Allow revert to saved snapshot if a divergence is detected."


Why is this a warning? If the user set this flag, it is not something unexpected. This can be a utl::info, but I'm not sure if it is necessary at all.

Other existing messages in gpl/src/replace.tcl tend to be warnings also.

I think this message is useful to make sure the non-default behavior is activated. Maybe let's leave it as ult::info?

I'm ok with utl::info.

eder-matheus · 2025-01-23T16:49:20Z

src/gpl/src/replace.cpp

@@ -198,7 +199,7 @@ void Replace::doIncrementalPlace(int threads)
  int iter = doNesterovPlace(threads);
  setNesterovPlaceMaxIter(previous_max_iter);

-  // Finish the overflow resolution from the rough placement
+  // Finish' the overflow resolution from the rough placement


Signed-off-by: Augusto Berndt <[email protected]>

github-actions

clang-tidy made some suggestions

github-actions · 2025-01-23T17:21:59Z

src/gpl/src/nesterovPlace.cpp

+                   diverge_snapshot_iter_,
+                   diverge_snapshot_average_overflow_unscaled_,
+                   min_hpwl_);
+        curA = diverge_snapshotA;


warning: Value stored to 'curA' is never read [clang-analyzer-deadcode.DeadStores]

curA = diverge_snapshotA; ^

Additional context

src/gpl/src/nesterovPlace.cpp:570: Value stored to 'curA' is never read

curA = diverge_snapshotA; ^

Signed-off-by: Augusto Berndt <[email protected]>

github-actions

clang-tidy made some suggestions

github-actions · 2025-01-23T17:46:10Z

src/gpl/src/nesterovPlace.cpp

  bool isDivergeTriedRevert = false;

+  // divergence snapshot info
+  float diverge_snapshotA = 0;


warning: variable 'diverge_snapshotA' set but not used [clang-diagnostic-unused-but-set-variable]

float diverge_snapshotA = 0; ^

github-actions · 2025-01-23T17:46:10Z

src/gpl/src/nesterovPlace.cpp

+      if (is_min_hpwl_) {
+        diverge_snapshotWlCoefX = wireLengthCoefX_;
+        diverge_snapshotWlCoefY = wireLengthCoefY_;
+        diverge_snapshotA = curA;


warning: Value stored to 'diverge_snapshotA' is never read [clang-analyzer-deadcode.DeadStores]

diverge_snapshotA = curA; ^

Additional context

src/gpl/src/nesterovPlace.cpp:419: Value stored to 'diverge_snapshotA' is never read

diverge_snapshotA = curA; ^

Signed-off-by: Augusto Berndt <[email protected]>

github-actions · 2025-01-23T18:23:20Z

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

eder-matheus

LGTM, but I'll wait for @maliberty's comments on the changes before merging.

maliberty

Fine aside from flipping the default

maliberty · 2025-01-21T23:33:56Z

src/gpl/src/replace.cpp

@@ -198,7 +199,7 @@ void Replace::doIncrementalPlace(int threads)
  int iter = doNesterovPlace(threads);
  setNesterovPlaceMaxIter(previous_max_iter);

-  // Finish the overflow resolution from the rough placement
+  // Finish' the overflow resolution from the rough placement


stray character

maliberty · 2025-01-21T23:34:54Z

src/gpl/src/nesterovPlace.cpp

+        // log_->report("[NesterovSolve] Snapshot for divergence saved at iter =
+        // {}", iter);


maliberty · 2025-01-23T21:37:28Z

src/gpl/README.md

@@ -97,6 +97,7 @@ global_placement
    [-timing_driven_net_weight_max]
    [-timing_driven_nets_percentage]
    [-keep_resize_below_overflow]
+    [-allow_revert_if_diverge]


We are going to flip this to on by default with a disable.

…e is detected Signed-off-by: Augusto Berndt <[email protected]>

Signed-off-by: Augusto Berndt <[email protected]>

github-actions · 2025-01-24T14:52:39Z

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

github-actions · 2025-01-24T14:58:38Z

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

gudeh added 8 commits January 16, 2025 17:59

gpl: revert to min overflow if divergence occured

1b72d58

Signed-off-by: Augusto Berndt <[email protected]>

gpl: use min hpwl for divergence snapshot

7f91020

adjust some routability messages Signed-off-by: Augusto Berndt <[email protected]>

gpl: remove mistaken modification in comment

60fd26c

Signed-off-by: Augusto Berndt <[email protected]>

gpl: include new TCL flag to control the allowance of revert if diverge

bacb646

Signed-off-by: Augusto Berndt <[email protected]>

gpl: invert bool with TCL flag

99a5025

Signed-off-by: Augusto Berndt <[email protected]>

Merge remote-tracking branch 'private/master' into gpl-revert-if-diverge

c0e0f78

gpl: update routability tests

3432b5e

Signed-off-by: Augusto Berndt <[email protected]>

Merge remote-tracking branch 'private/master' into gpl-revert-if-diverge

e43a0b7

openroad-ci assigned gudeh Jan 20, 2025

github-actions bot reviewed Jan 20, 2025

View reviewed changes

gudeh added 2 commits January 20, 2025 21:40

gpl: edit README with new command

f3dfd6d

Signed-off-by: Augusto Berndt <[email protected]>

gpl: fix setting allow revert if diverge

492e7f0

Signed-off-by: Augusto Berndt <[email protected]>

oharboe reviewed Jan 21, 2025

View reviewed changes

src/gpl/src/replace.tcl Outdated Show resolved Hide resolved

Merge remote-tracking branch 'private/master' into gpl-revert-if-diverge

1442c07

openroad-ci mentioned this pull request Jan 21, 2025

gpl: activate revert if diverge for nangate45/bp_be The-OpenROAD-Project/OpenROAD-flow-scripts#2700

Closed

gudeh requested review from maliberty and eder-matheus January 21, 2025 19:17

Merge remote-tracking branch 'private/master' into gpl-revert-if-diverge

5879308

gudeh added 2 commits January 22, 2025 14:50

Merge remote-tracking branch 'private/master' into gpl-revert-if-diverge

ea3dc03

Merge remote-tracking branch 'private/master' into gpl-revert-if-diverge

170fa4b

eder-matheus requested changes Jan 23, 2025

View reviewed changes

gudeh added 2 commits January 23, 2025 17:06

gpl: fix typos, modify message type

b1272df

Signed-off-by: Augusto Berndt <[email protected]>

Merge remote-tracking branch 'private/master' into gpl-revert-if-diverge

f01415c

github-actions bot reviewed Jan 23, 2025

View reviewed changes

gpl: remove unusemodification to never used variable

a9a9d73

Signed-off-by: Augusto Berndt <[email protected]>

github-actions bot reviewed Jan 23, 2025

View reviewed changes

gpl: remove unused variable

76bba71

Signed-off-by: Augusto Berndt <[email protected]>

eder-matheus approved these changes Jan 23, 2025

View reviewed changes

maliberty reviewed Jan 23, 2025

View reviewed changes

gudeh added 8 commits January 23, 2025 22:05

gpl: invert default behavior for reverting to snpashot when divergenc…

a3baff2

…e is detected Signed-off-by: Augusto Berndt <[email protected]>

Merge remote-tracking branch 'private/master' into gpl-revert-if-diverge

b273e56

Merge remote-tracking branch 'private/master' into gpl-revert-if-diverge

7236b71

gpl: invert allo revert in README, fix broekn replace.tcl

cf2e9d9

Signed-off-by: Augusto Berndt <[email protected]>

Merge remote-tracking branch 'private/master' into gpl-revert-if-diverge

53397c4

gpl: invert default logic for revert if diverge

4893563

Signed-off-by: Augusto Berndt <[email protected]>

gpl: minor formating changes

4d54af6

Signed-off-by: Augusto Berndt <[email protected]>

gpl: clang-format

3a40747

Signed-off-by: Augusto Berndt <[email protected]>

gudeh requested a review from maliberty January 24, 2025 15:01

eder-matheus merged commit 1ab5157 into The-OpenROAD-Project:master Jan 24, 2025
11 checks passed

openroad-ci deleted the gpl-revert-if-diverge branch January 24, 2025 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gpl: revert if diverge #6561

gpl: revert if diverge #6561

openroad-ci commented Jan 20, 2025

github-actions bot left a comment

github-actions bot Jan 20, 2025

oharboe commented Jan 21, 2025

gudeh commented Jan 21, 2025

gudeh commented Jan 21, 2025

gudeh commented Jan 22, 2025 •

edited

Loading

oharboe commented Jan 22, 2025

eder-matheus Jan 23, 2025

eder-matheus Jan 23, 2025

gudeh Jan 23, 2025

eder-matheus Jan 23, 2025

eder-matheus Jan 23, 2025

github-actions bot left a comment

github-actions bot Jan 23, 2025

github-actions bot left a comment

github-actions bot Jan 23, 2025

github-actions bot Jan 23, 2025

github-actions bot commented Jan 23, 2025

eder-matheus left a comment

maliberty left a comment

maliberty Jan 21, 2025

maliberty Jan 21, 2025

maliberty Jan 23, 2025

github-actions bot commented Jan 24, 2025

github-actions bot commented Jan 24, 2025

		// log_->report("[NesterovSolve] Snapshot for divergence saved at iter =
		// {}", iter);

		utl::warn "GPL" 153 \
		"Allow revert to saved snapshot if a divergence is detected."

gpl: revert if diverge #6561

gpl: revert if diverge #6561

Conversation

openroad-ci commented Jan 20, 2025

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jan 20, 2025

Choose a reason for hiding this comment

oharboe commented Jan 21, 2025

gudeh commented Jan 21, 2025

gudeh commented Jan 21, 2025

gudeh commented Jan 22, 2025 • edited Loading

oharboe commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jan 23, 2025

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jan 23, 2025

Choose a reason for hiding this comment

github-actions bot Jan 23, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 23, 2025

eder-matheus left a comment

Choose a reason for hiding this comment

maliberty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 24, 2025

github-actions bot commented Jan 24, 2025

gudeh commented Jan 22, 2025 •

edited

Loading