Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/gpl/src/nesterovBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1099,6 +1099,12 @@ NesterovBaseCommon::NesterovBaseCommon(
gNetStor_.reserve(pbc_->getNets().size());
for (auto& net : pbc_->getNets()) {
GNet myGNet(net);
// Read custom placement weight from ODB property (set by set_net_weight)
odb::dbDoubleProperty* wp
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would be nice to have better names for variables, for example renaming wp and myGNet. Perhaps net_property, and temp_gnet?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most importantly, the setting of timing-driven weights does not originally happen here. This is currently not protected by timing-driven mode being activated.

I believe this should happen at:

bool TimingBase::executeTimingDriven(bool run_journal_restore)

Notice how we originally set weights at the following, after RSZ is executed:

gNet->setTimingWeight(weight);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is an interesting new feature!

= odb::dbDoubleProperty::find(net->getDbNet(), "gpl_weight");
if (wp) {
myGNet.setCustomWeight(static_cast<float>(wp->getValue()));
}
Comment on lines +1103 to +1107
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To improve maintainability and avoid magic strings, it's a good practice to define property names as constants. This also allows for a more concise check using an if statement with an initializer.

A similar change should be applied in src/gpl/src/replace.i where this property is set.

    static const char* gpl_weight_prop_name = "gpl_weight";
    if (auto* wp = odb::dbDoubleProperty::find(net->getDbNet(), gpl_weight_prop_name)) {
      myGNet.setCustomWeight(static_cast<float>(wp->getValue()));
    }
References
  1. Define tunable parameters as named constants instead of using hardcoded magic numbers.

gNetStor_.push_back(myGNet);
}

Expand Down
32 changes: 32 additions & 0 deletions src/gpl/src/replace.i
Original file line number Diff line number Diff line change
Expand Up @@ -198,4 +198,36 @@ set_debug_cmd(int pause_iterations,
generate_images, resolved_path);
}

void
set_net_weight_cmd(const char* net_name, float weight)
{
auto* block = ord::OpenRoad::openRoad()->getDb()->getChip()->getBlock();
auto* net = block->findNet(net_name);
if (!net) {
ord::OpenRoad::openRoad()->getLogger()->error(
utl::GPL, 500, "Net '{}' not found.", net_name);
}
// Store as persistent ODB property so it survives write_db/read_db
auto* existing = odb::dbDoubleProperty::find(net, "gpl_weight");
if (existing) {
odb::dbDoubleProperty::destroy(existing);
}
odb::dbDoubleProperty::create(net, "gpl_weight", weight);
Comment on lines +206 to +215
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

There are a couple of improvements that can be made here:

  1. Robustness: While logger->error is expected to terminate the program, the code proceeds to dereference net on the following lines, which will cause a crash if net is null and termination does not occur. It's safer to add a return statement after calling error.
  2. Maintainability: To avoid magic strings, it's a good practice to define property names like "gpl_weight" as a constant.
  3. Efficiency: It's more efficient to update the property value if it exists, instead of destroying and recreating it.

The suggested code applies these improvements.

  if (!net) {
    ord::OpenRoad::openRoad()->getLogger()->error(
        utl::GPL, 500, "Net '{}' not found.", net_name);
    return;
  }
  // Store as persistent ODB property so it survives write_db/read_db
  static const char* gpl_weight_prop_name = "gpl_weight";
  if (auto* prop = odb::dbDoubleProperty::find(net, gpl_weight_prop_name)) {
    prop->setValue(weight);
  } else {
    odb::dbDoubleProperty::create(net, gpl_weight_prop_name, weight);
  }
References
  1. Define tunable parameters as named constants instead of using hardcoded magic numbers.

}

void
unset_net_weight_cmd(const char* net_name)
{
auto* block = ord::OpenRoad::openRoad()->getDb()->getChip()->getBlock();
auto* net = block->findNet(net_name);
if (!net) {
ord::OpenRoad::openRoad()->getLogger()->error(
utl::GPL, 501, "Net '{}' not found.", net_name);
}
auto* existing = odb::dbDoubleProperty::find(net, "gpl_weight");
if (existing) {
odb::dbDoubleProperty::destroy(existing);
}
Comment on lines +223 to +230
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

There are a couple of improvements that can be made here:

  1. Robustness: While logger->error is expected to terminate, the code proceeds to dereference net on the following lines, which will cause a crash if net is null and termination does not occur. It's safer to add a return statement after calling error.
  2. Maintainability: To avoid magic strings, it's a good practice to define property names like "gpl_weight" as a constant.

The suggested code applies these improvements and uses a more concise if statement with an initializer.

  if (!net) {
    ord::OpenRoad::openRoad()->getLogger()->error(
        utl::GPL, 501, "Net '{}' not found.", net_name);
    return;
  }
  static const char* gpl_weight_prop_name = "gpl_weight";
  if (auto* existing = odb::dbDoubleProperty::find(net, gpl_weight_prop_name)) {
    odb::dbDoubleProperty::destroy(existing);
  }
References
  1. Define tunable parameters as named constants instead of using hardcoded magic numbers.

}

%} // inline
26 changes: 26 additions & 0 deletions src/gpl/src/replace.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -256,3 +256,29 @@ proc parse_inst_names { cmd names } {
return $inst_list
}
}

sta::define_cmd_args "set_net_weight" {net_name weight}

proc set_net_weight { args } {
sta::check_argc_eq2 "set_net_weight" $args
if { [ord::get_db_block] == "NULL" } {
utl::error GPL 503 "No design block found."
}
set net_name [lindex $args 0]
set weight [lindex $args 1]
if {![string is double -strict $weight] || $weight <= 0.0 || $weight != $weight} {
utl::error GPL 502 "Weight '$weight' must be a finite positive number > 0."
}
gpl::set_net_weight_cmd $net_name $weight
}

sta::define_cmd_args "unset_net_weight" {net_name}

proc unset_net_weight { args } {
sta::check_argc_eq1 "unset_net_weight" $args
if { [ord::get_db_block] == "NULL" } {
utl::error GPL 504 "No design block found."
}
set net_name [lindex $args 0]
gpl::unset_net_weight_cmd $net_name
}
2 changes: 2 additions & 0 deletions src/gpl/test/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ TESTS = [
"simple09",
"simple10",
"region01",
"net_weight01",
"net_weight_negative",
]

py_library(
Expand Down
2 changes: 2 additions & 0 deletions src/gpl/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ or_integration_tests(
simple09
simple10
region01
net_weight01
net_weight_negative
)


Expand Down
88 changes: 88 additions & 0 deletions src/gpl/test/net_weight01.ok
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
[INFO ODB-0227] LEF file: ./nangate45.lef, created 22 layers, 27 vias, 134 library cells
[INFO ODB-0128] Design: gcd
[INFO ODB-0130] Created 54 pins.
[INFO ODB-0131] Created 294 components and 1656 component-terminals.
[INFO ODB-0133] Created 364 nets and 1068 connections.
Test net: resp_msg\[9\]
PASS: set_net_weight resp_msg\[9\] 5.0
PASS: property value = 5.0
PASS: unset_net_weight
PASS: multiple nets weighted
[INFO GPL-0001] ---- Initialize GPL Main Data Structures
[INFO GPL-0002] DBU: 2000
[INFO GPL-0003] SiteSize: ( 0.190 1.400 ) um
[INFO GPL-0004] CoreBBox: ( 0.000 0.000 ) ( 30.970 30.800 ) um
[INFO GPL-0036] Movable instances area: 569.772 um^2
[INFO GPL-0037] Total instances area: 569.772 um^2
[INFO GPL-0035] Pin density area adjust: 49.963 um^2
[INFO GPL-0032] ---- Initialize Region: Top-level
[INFO GPL-0006] Number of instances: 294
[INFO GPL-0007] Movable instances: 294
[INFO GPL-0008] Fixed instances: 0
[INFO GPL-0009] Dummy instances: 0
[INFO GPL-0010] Number of nets: 364
[INFO GPL-0011] Number of pins: 1122
[INFO GPL-0012] Die BBox: ( 0.000 0.000 ) ( 30.970 30.800 ) um
[INFO GPL-0013] Core BBox: ( 0.000 0.000 ) ( 30.970 30.800 ) um
[INFO GPL-0016] Core area: 953.876 um^2
[INFO GPL-0014] Region name: top-level.
[INFO GPL-0015] Region area: 953.876 um^2
[INFO GPL-0017] Fixed instances area: 0.000 um^2
[INFO GPL-0018] Movable instances area: 619.735 um^2
[INFO GPL-0019] Utilization: 64.970 %
[INFO GPL-0020] Standard cells area: 619.735 um^2
[INFO GPL-0021] Large instances area: 0.000 um^2
[INFO GPL-0005] ---- Execute Conjugate Gradient Initial Placement.
[INFO GPL-0051] Source of initial instance position counters:
Odb location = 0 Core center = 294 Region center = 0
[INFO GPL-0033] ---- Initialize Nesterov Region: Top-level
[INFO GPL-0023] Placement target density: 0.7000
[INFO GPL-0024] Movable insts average area: 2.108 um^2
[INFO GPL-0025] Ideal bin area: 3.011 um^2
[INFO GPL-0026] Ideal bin count: 316
[INFO GPL-0027] Total bin area: 953.876 um^2
[INFO GPL-0028] Bin count (X, Y): 16 , 16
[INFO GPL-0029] Bin size (W * H): 1.936 * 1.925 um
[INFO GPL-0030] Number of bins: 256
[INFO GPL-0007] ---- Execute Nesterov Global Placement.
[INFO GPL-0031] HPWL: Half-Perimeter Wirelength
Iteration | Overflow | HPWL (um) | HPWL(%) | Penalty | Group
---------------------------------------------------------------
0 | 0.8993 | 1.826026e+03 | +0.00% | 1.07e-11 |
10 | 0.7518 | 2.159992e+03 | +18.29% | 1.75e-11 |
20 | 0.7369 | 2.161174e+03 | +0.05% | 2.85e-11 |
30 | 0.7377 | 2.156581e+03 | -0.21% | 4.64e-11 |
40 | 0.7357 | 2.162750e+03 | +0.29% | 7.55e-11 |
50 | 0.7314 | 2.171305e+03 | +0.40% | 1.23e-10 |
60 | 0.7253 | 2.183808e+03 | +0.58% | 2.00e-10 |
70 | 0.7136 | 2.201126e+03 | +0.79% | 3.26e-10 |
80 | 0.6966 | 2.225340e+03 | +1.10% | 5.32e-10 |
90 | 0.6779 | 2.257155e+03 | +1.43% | 8.66e-10 |
100 | 0.6536 | 2.299164e+03 | +1.86% | 1.41e-09 |
110 | 0.6209 | 2.347675e+03 | +2.11% | 2.30e-09 |
120 | 0.5828 | 2.390463e+03 | +1.82% | 3.74e-09 |
130 | 0.5399 | 2.441236e+03 | +2.12% | 6.10e-09 |
140 | 0.4867 | 2.460689e+03 | +0.80% | 9.93e-09 |
150 | 0.4299 | 2.462688e+03 | +0.08% | 1.62e-08 |
160 | 0.3764 | 2.479178e+03 | +0.67% | 2.63e-08 |
170 | 0.3303 | 2.488403e+03 | +0.37% | 4.12e-08 |
180 | 0.2954 | 2.509546e+03 | +0.85% | 6.07e-08 |
190 | 0.2662 | 2.539786e+03 | +1.20% | 8.95e-08 |
200 | 0.2373 | 2.560392e+03 | +0.81% | 1.32e-07 |
210 | 0.2109 | 2.580240e+03 | +0.78% | 1.94e-07 |
220 | 0.1782 | 2.599993e+03 | +0.77% | 2.86e-07 |
230 | 0.1425 | 2.613456e+03 | +0.52% | 4.21e-07 |
240 | 0.1186 | 2.629106e+03 | +0.60% | 6.21e-07 |
247 | 0.0998 | 2.637883e+03 | | 8.46e-07 |
---------------------------------------------------------------
[INFO GPL-1001] Global placement finished at iteration 247
[INFO GPL-1002] Placed Cell Area 619.7347
[INFO GPL-1003] Available Free Area 953.8760
[INFO GPL-1004] Minimum Feasible Density 0.6500 (cell_area / free_area)
[INFO GPL-1006] Suggested Target Densities:
[INFO GPL-1007] - For 90% usage of free space: 0.7219
[INFO GPL-1008] - For 80% usage of free space: 0.8121
[INFO GPL-1014] Final placement area: 619.73 (+0.00%)
PASS: global_placement with net weights
PASS: overwrite weight
PASS: all tests passed
57 changes: 57 additions & 0 deletions src/gpl/test/net_weight01.tcl
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# Test set_net_weight / unset_net_weight
source helpers.tcl
set test_name net_weight01
read_lef ./nangate45.lef
read_def ./simple01.def

# Test 1: set_net_weight on existing net
set nets [odb::dbBlock_getNets [ord::get_db_block]]
set first_net [lindex $nets 0]
set net_name [$first_net getName]
puts "Test net: $net_name"

set_net_weight $net_name 5.0
puts "PASS: set_net_weight $net_name 5.0"

# Test 2: verify property exists in ODB
set prop [odb::dbDoubleProperty_find $first_net "gpl_weight"]
if {$prop == "NULL"} {
error "FAIL: gpl_weight property not found"
}
set val [$prop getValue]
if {abs($val - 5.0) > 0.01} {
error "FAIL: expected 5.0, got $val"
}
puts "PASS: property value = $val"

# Test 3: unset
unset_net_weight $net_name
set prop2 [odb::dbDoubleProperty_find $first_net "gpl_weight"]
if {$prop2 != "NULL"} {
error "FAIL: property should be removed"
}
puts "PASS: unset_net_weight"

# Test 4: set multiple nets
set_net_weight $net_name 3.0
set second_net [lindex $nets 1]
set net2_name [$second_net getName]
set_net_weight $net2_name 7.0
puts "PASS: multiple nets weighted"

# Test 5: run global_placement with weights (functional test)
global_placement -init_density_penalty 0.01 -skip_initial_place
puts "PASS: global_placement with net weights"

# Test 6: overwrite weight
set_net_weight $net_name 10.0
set prop3 [odb::dbDoubleProperty_find $first_net "gpl_weight"]
set val3 [$prop3 getValue]
if {abs($val3 - 10.0) > 0.01} {
error "FAIL: overwrite expected 10.0, got $val3"
}
puts "PASS: overwrite weight"

# Test 7: write_db / read_db persistence
write_db [make_result_file "${test_name}.odb"]
puts "PASS: all tests passed"
28 changes: 28 additions & 0 deletions src/gpl/test/net_weight_negative.ok
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
[INFO ODB-0227] LEF file: ./nangate45.lef, created 22 layers, 27 vias, 134 library cells
[INFO ODB-0128] Design: gcd
[INFO ODB-0130] Created 54 pins.
[INFO ODB-0131] Created 294 components and 1656 component-terminals.
[INFO ODB-0133] Created 364 nets and 1068 connections.
=== Test 1: non-existent net ===
[ERROR GPL-0500] Net 'nonexistent_net' not found.
PASS: caught error: GPL-0500
=== Test 2: zero weight ===
[ERROR GPL-0502] weight '0.0' must be a finite positive number > 0.
PASS: caught error for zero weight: GPL-0502
=== Test 3: negative weight ===
[ERROR GPL-0502] weight '-1.0' must be a finite positive number > 0.
PASS: caught error for negative weight: GPL-0502
=== Test 4: unset non-existent net ===
[ERROR GPL-0501] Net 'nonexistent_net' not found.
PASS: caught error: GPL-0501
=== Test 5: wrong arg count ===
[ERROR STA-0567] set_net_weight requires two positional arguments.
PASS: caught error for no args: STA-0567
[ERROR STA-0567] set_net_weight requires two positional arguments.
PASS: caught error for 3 args: STA-0567
=== Test 6: non-numeric weight ===
[ERROR GPL-0502] weight 'abc' must be a finite positive number > 0.
PASS: caught error for non-numeric: GPL-0502
=== Test 7: unset net without weight ===
PASS: unset on net without weight (no-op)
=== All negative tests passed ===
69 changes: 69 additions & 0 deletions src/gpl/test/net_weight_negative.tcl
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
source helpers.tcl
set test_name net_weight_negative
read_lef ./nangate45.lef
read_def ./simple01.def

# Negative Test 1: non-existent net
puts "=== Test 1: non-existent net ==="
if {[catch {set_net_weight nonexistent_net 5.0} err]} {
puts "PASS: caught error: $err"
} else {
error "FAIL: should have thrown error for non-existent net"
}

# Negative Test 2: zero weight
puts "=== Test 2: zero weight ==="
if {[catch {set_net_weight clk 0.0} err]} {
puts "PASS: caught error for zero weight: $err"
} else {
error "FAIL: should reject zero weight"
}

# Negative Test 3: negative weight
puts "=== Test 3: negative weight ==="
if {[catch {set_net_weight clk -1.0} err]} {
puts "PASS: caught error for negative weight: $err"
} else {
error "FAIL: should reject negative weight"
}

# Negative Test 4: unset non-existent net
puts "=== Test 4: unset non-existent net ==="
if {[catch {unset_net_weight nonexistent_net} err]} {
puts "PASS: caught error: $err"
} else {
error "FAIL: should have thrown error for non-existent net"
}

# Negative Test 5: wrong number of arguments
puts "=== Test 5: wrong arg count ==="
if {[catch {set_net_weight} err]} {
puts "PASS: caught error for no args: $err"
} else {
error "FAIL: should reject no args"
}

if {[catch {set_net_weight a b c} err]} {
puts "PASS: caught error for 3 args: $err"
} else {
error "FAIL: should reject 3 args"
}

# Negative Test 6: non-numeric weight
puts "=== Test 6: non-numeric weight ==="
if {[catch {set_net_weight clk abc} err]} {
puts "PASS: caught error for non-numeric: $err"
} else {
error "FAIL: should reject non-numeric weight"
}

# Negative Test 7: unset net that has no weight (should not crash)
puts "=== Test 7: unset net without weight ==="
set nets [odb::dbBlock_getNets [ord::get_db_block]]
set first_net [lindex $nets 0]
set net_name [$first_net getName]
# Don't set weight, just unset — should be no-op, no crash
unset_net_weight $net_name
puts "PASS: unset on net without weight (no-op)"

puts "=== All negative tests passed ==="