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
23 changes: 11 additions & 12 deletions src/gpl/src/mbff.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -927,27 +927,26 @@ void MBFF::ModifyPinConnections(const std::vector<Flop>& flops,
}

// Store original FF→tray pin mapping as a property on the tray
// instance so timing reports can display the original pin name.
std::string tray_port;
// pin (iterm) so the report_path "orig_name" field can display the
// original pin name.
dbITerm* tray_iterm = nullptr;
if (is_d && d_pin) {
tray_port = d_pin->name();
tray_iterm = tray_inst[tray_idx]->findITerm(d_pin->name().c_str());
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.

high

The sta::LibertyPort::name() method returns a const char*. Calling .c_str() on a raw pointer is a compilation error. You should pass the result of name() directly to findITerm.

tray_iterm = tray_inst[tray_idx]->findITerm(d_pin->name());

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Gemini you are wrong. LibertyPort inherits ConcretePort::name() which returns const std::string & . So c_str() on it is valid and necessary because dbInst::findITerm takes const char*

Current code:
tray_iterm = tray_inst[tray_idx]->findITerm(d_pin->name().c_str());

is correct,
Without .c_str() compile would fail (can't convert const std::string& to const char* implicitly).

} else if (is_q) {
if (is_qn_inv && qn_pin) {
tray_port = qn_pin->name();
} else if (q_pin) {
tray_port = q_pin->name();
const sta::LibertyPort* tray_port = is_qn_inv ? qn_pin : q_pin;
if (tray_port) {
tray_iterm
= tray_inst[tray_idx]->findITerm(tray_port->name().c_str());
}
}
Comment on lines +932 to 941
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

The logic for selecting tray_iterm for Q/QN pins can be simplified and made more robust. The current implementation might incorrectly fall back to a non-inverting pin (q_pin) if an inverting pin (qn_pin) is expected but missing in the tray, which would lead to misleading "Orig Name" reports. Additionally, consider pre-fetching the tray iterms for the current slot outside the iterm loop to avoid redundant findITerm calls (string lookups) for each pin of the original flop.

      dbITerm* tray_iterm = nullptr;
      if (is_d && d_pin) {
        tray_iterm = tray_inst[tray_idx]->findITerm(d_pin->name().c_str());
      } else if (is_q) {
        const sta::LibertyPort* tray_port = is_qn_inv ? qn_pin : q_pin;
        if (tray_port) {
          tray_iterm = tray_inst[tray_idx]->findITerm(tray_port->name().c_str());
        }
      }
References
  1. When analyzing code within a loop, consider the entire loop structure to identify and remove redundant operations inside the loop body.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I addressed this

if (!tray_port.empty()) {
const std::string key = "orig_name_" + tray_port;
if (tray_iterm) {
const std::string val = orig_inst_name + "/" + orig_port_name;
odb::dbStringProperty* prop
= odb::dbStringProperty::find(tray_inst[tray_idx], key.c_str());
= odb::dbStringProperty::find(tray_iterm, kOrigNameProp);
if (prop) {
prop->setValue(val.c_str());
} else {
odb::dbStringProperty::create(
tray_inst[tray_idx], key.c_str(), val.c_str());
odb::dbStringProperty::create(tray_iterm, kOrigNameProp, val.c_str());
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/gpl/src/mbff.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ class Scene;

namespace gpl {

inline constexpr const char* kOrigNameProp = "orig_name";

struct Point;
struct Tray;
struct Flop;
Expand Down
38 changes: 38 additions & 0 deletions src/gpl/src/replace.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <utility>

#include "AbstractGraphics.h"
#include "db_sta/dbNetwork.hh"
#include "db_sta/dbSta.hh"
#include "graphicsNone.h"
#include "initialPlace.h"
Expand All @@ -21,7 +22,10 @@
#include "placerBase.h"
#include "routeBase.h"
#include "rsz/Resizer.hh"
#include "sta/Graph.hh"
#include "sta/Path.hh"
#include "sta/StaMain.hh"
#include "sta/StaState.hh"
#include "timingBase.h"
#include "utl/Logger.h"
#include "utl/validation.h"
Expand All @@ -38,6 +42,40 @@ Replace::Replace(odb::dbDatabase* odb,
: db_(odb), sta_(sta), rs_(resizer), fr_(router), log_(logger)
{
graphics_ = std::make_unique<GraphicsNone>();

// Register "orig_name" report_path field: original pin name before
// multi-bit clustering. Reads "orig_name" property off the path
// vertex's iterm. Registered at tool init so paths can be reported
// even when the design is loaded from an already-clustered .odb
// without re-running cluster_flops.
if (sta_->findReportPathField(kOrigNameProp) == nullptr) {
sta::dbNetwork* network = sta_->getDbNetwork();
sta_->makeReportPathField(
kOrigNameProp,
kOrigNameProp,
"Orig Name",
36,
true,
nullptr,
[network](const sta::Path* path,
const sta::StaState* sta) -> std::string {
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.

warning: no header providing "sta::StaState" is directly included [misc-include-cleaner]

src/gpl/src/replace.cpp:27:

- #include "timingBase.h"
+ #include "sta/StaState.hh"
+ #include "timingBase.h"

if (path == nullptr) {
return {};
}
const sta::Pin* pin = path->vertex(sta)->pin();
// staToDb requires all three out-params; only iterm is used.
odb::dbITerm* iterm = nullptr;
odb::dbBTerm* bterm = nullptr;
odb::dbModITerm* moditerm = nullptr;
network->staToDb(pin, iterm, bterm, moditerm);
if (iterm == nullptr) {
return {};
}
odb::dbStringProperty* prop
= odb::dbStringProperty::find(iterm, kOrigNameProp);
return prop ? prop->getValue() : std::string{};
Comment on lines +74 to +76
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Fall back to old instance-level orig_name properties

When users load an ODB written by the previous implementation after cluster_flops, the saved mappings are still on the tray dbInst under orig_name_<port>, but this new getter only looks for orig_name on the dbITerm. That makes report_checks -fields {orig_name} silently blank out existing clustered databases even though the old data is present; please preserve compatibility by falling back to the instance property using the current iterm's port name when the pin-level property is missing.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is factually correct about behavior: old .odb files written before this PR have orig_name_ on the tray dbInst (commit 649366b). New getter reads orig_name on dbITerm --> silent blank for those legacy .odb files. But the orig name storage was a recent April 2026 commit, not a long-standing release. It will be better to rerun cluster_flops to regenerate new-format properties automatically.

});
}
}

Replace::~Replace() = default;
Expand Down
41 changes: 0 additions & 41 deletions src/gpl/test/mbff_orig_name.def

This file was deleted.

19 changes: 12 additions & 7 deletions src/gpl/test/mbff_orig_name.ok
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,23 @@
[INFO ODB-0227] LEF file: ./4BitTrayH4/asap7sc7p5t_DFFHQNV4X.lef, created 9 library cells
[INFO ODB-0394] Duplicate site asap7sc7p5t_pg in asap7sc7p5t_DFFHQNH2V2X already seen in asap7sc7p5t_DFFHQNV2X
[INFO ODB-0227] LEF file: ./4BitTrayH2W2/asap7sc7p5t_DFFHQNH2V2X.lef, created 9 library cells
[INFO ODB-0128] Design: tray_test
[INFO ODB-0130] Created 9 pins.
[INFO ODB-0131] Created 4 components and 20 component-terminals.
[INFO ODB-0133] Created 9 nets and 12 connections.
orig_name field registered at session init (pre-cluster_flops).
[WARNING IFP-0028] Core area lower left (1.000, 1.000) snapped to (1.026, 1.080).
[INFO IFP-0001] Added 29 rows of 147 site asap7sc7p5t.
[INFO IFP-0100] Die BBox: ( 0.000 0.000 ) ( 10.000 10.000 ) um
[INFO IFP-0101] Core BBox: ( 1.026 1.080 ) ( 8.964 8.910 ) um
[INFO IFP-0102] Core area: 62.155 um^2
[INFO IFP-0103] Total instances area: 1.166 um^2
[INFO IFP-0104] Effective utilization: 0.019
[INFO IFP-0105] Number of instances: 4
Alpha = 40.0, Beta = 1.0, #paths = 0, max size = -1
Total ILP Cost: 112.643
Total Timing Critical Path Displacement: 0.0
Average slot-to-flop displacement: 1.730
Final Objective Value: 112.643
Sizes used
4-bit: 1
Startpoint: d3 (input port clocked by clk)
Startpoint: d4 (input port clocked by clk)
Endpoint: _tray_size4_7 (rising edge-triggered flip-flop clocked by clk)
Path Group: clk
Path Type: max
Expand All @@ -26,8 +31,8 @@ Path Type: max
0.00 0.00 clock clk (rise edge)
0.00 0.00 clock network delay (ideal)
0.00 0.00 ^ input external delay
0.00 0.00 ^ d3 (in)
0.00 0.00 ^ _tray_size4_7/D1 (DFFHQNV4Xx1_ASAP7_75t_L) ff3/D
0.00 0.00 ^ d4 (in)
0.00 0.00 ^ _tray_size4_7/D0 (DFFHQNV4Xx1_ASAP7_75t_L) ff4/D
0.00 data arrival time

1000.00 1000.00 clock clk (rise edge)
Expand Down
29 changes: 25 additions & 4 deletions src/gpl/test/mbff_orig_name.tcl
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,26 @@ read_lib ./4BitTrayH4/asap7sc7p5t_DFFHQNV4X_LVT_TT_nldm_FAKE.lib
read_lef ./4BitTrayH2W2/asap7sc7p5t_DFFHQNH2V2X.lef
read_lib ./4BitTrayH2W2/asap7sc7p5t_DFFHQNH2V2X_LVT_TT_nldm_FAKE.lib

read_def ./$test_name.def
read_verilog ./$test_name.v
link_design tray_test

# Verify the "orig_name" report field is registered at session init
# (Replace ctor), not deferred until cluster_flops. Required so an
# already-clustered .odb loaded in a fresh session can still report
# the saved orig_name properties via report_checks.
if { [sta::find_report_path_field_abrev orig_name] == "" } {
utl::error GPL 330 "orig_name report_path field not registered at session init."
}
puts "orig_name field registered at session init (pre-cluster_flops)."

initialize_floorplan -die_area "0 0 10 10" \
-core_area "1 1 9 9" \
-site asap7sc7p5t

place_inst -name ff1 -origin {6 6} -status PLACED
place_inst -name ff2 -origin {4 6} -status PLACED
place_inst -name ff3 -origin {4 4} -status PLACED
place_inst -name ff4 -origin {6 4} -status PLACED

create_clock -name clk -period 1000 [get_ports clk1]
set_input_delay -clock clk 0 [get_ports {d1 d2 d3 d4}]
Expand All @@ -24,6 +43,8 @@ cluster_flops -tray_weight 40.0 \
-max_split_size -1 \
-num_paths 0

# Report timing to verify original FF names appear in the path report.
# After clustering the tray pin descriptions should show in the Orig Name column.
report_checks -path_delay max -fields {orig_name} -through [get_pins _tray_size4_7/D1]
# Report timing to verify original FF pin names appear in the path
# report. After clustering the tray pin descriptions show the ffN/D
# mapping in the Orig Name column.
report_checks -path_delay max -fields {orig_name} \
-through [get_pins -of_objects [get_cells _tray_size4_*]]
16 changes: 16 additions & 0 deletions src/gpl/test/mbff_orig_name.v
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
module tray_test (clk1, d1, d2, d3, d4, o1, o2, o3, o4);
input clk1;
input d1;
input d2;
input d3;
input d4;
output o1;
output o2;
output o3;
output o4;

DFFHQNx1_ASAP7_75t_L ff1 (.CLK(clk1), .D(d1), .QN(o1));
DFFHQNx1_ASAP7_75t_L ff2 (.CLK(clk1), .D(d2), .QN(o2));
DFFHQNx1_ASAP7_75t_L ff3 (.CLK(clk1), .D(d3), .QN(o3));
DFFHQNx1_ASAP7_75t_L ff4 (.CLK(clk1), .D(d4), .QN(o4));
endmodule
Loading