Merge pull request #8107 from The-OpenROAD-Project-staging/secure-hold-buffering-fix

rsz: Fixed RepairHold::makeHoldDelay() to resolve ORD-2030 errors in multiple hierarchical designs

Author: maliberty

src/dbSta/src/dbEditHierarchy.cc

@@ -236,12 +236,34 @@ void dbEditHierarchy::hierarchicalConnect(dbITerm* source_pin,
 
   dlogHierConnStart(source_pin, dest_pin, connection_name);
 
-  // 1. Get the scope (dbModule*) of the source and destination pins
-  dbModule* source_db_module = source_pin->getInst()->getModule();
-  dbModule* dest_db_module = dest_pin->getInst()->getModule();
-  // it is possible that one or other of the pins is not involved
-  // in hierarchy, which is ok, and the source/dest modnet will be null
-  dbModNet* source_db_mod_net = source_pin->getModNet();
+  //
+  // 1. Connect source and dest pins directly in flat flow
+  //
+  dbNet* source_db_net = source_pin->getNet();
+  dbNet* dest_db_net = dest_pin->getNet();
+  if (db_network_->hierarchy_ == false) {
+    // If both source pin and dest pin do not have a corresponding flat net,
+    // Create a new net and connect it with source pin.
+    if (source_db_net == nullptr && dest_db_net == nullptr) {
+      Net* new_net = db_network_->makeNet(
+          connection_name,
+          db_network_->parent(db_network_->dbToSta(source_pin->getInst())),
+          odb::dbNameUniquifyType::IF_NEEDED);
+      source_db_net = db_network_->staToDb(new_net);
+      source_pin->connect(source_db_net);
+    }
+
+    // Connect
+    if (source_db_net) {
+      dest_pin->connect(source_db_net);
+    } else {
+      assert(dest_db_net);
+      source_pin->connect(dest_db_net);
+    }
+
+    dlogHierConnDone();
+    return;  // Done here in a flat flow
+  }
 
   //
   // 2. Make sure there is a direct flat net connection
@@ -250,7 +272,6 @@ void dbEditHierarchy::hierarchicalConnect(dbITerm* source_pin,
   // co-existing, something we respect even when making
   // new hierarchical connections.
   //
-  dbNet* source_db_net = source_pin->getNet();
   if (!source_db_net) {
     dlogHierConnCreateFlatNet(connection_name);
     Net* new_net = db_network_->makeNet(
@@ -271,6 +292,13 @@ void dbEditHierarchy::hierarchicalConnect(dbITerm* source_pin,
   // 3. Make the hierarchical connection.
   // in case when pins in different modules
   //
+
+  // Get the scope (dbModule*) of the source and destination pins
+  dbModule* source_db_module = source_pin->getInst()->getModule();
+  dbModule* dest_db_module = dest_pin->getInst()->getModule();
+  // it is possible that one or other of the pins is not involved
+  // in hierarchy, which is ok, and the source/dest modnet will be null
+  dbModNet* source_db_mod_net = source_pin->getModNet();
   if (source_db_module != dest_db_module) {
     //
     // 3.1. Attempt to factor connection (minimize punch through), and return

src/dbSta/src/dbNetwork.cc

@@ -1578,6 +1578,7 @@ ObjectId dbNetwork::id(const Net* net) const
     const dbObject* obj = reinterpret_cast<const dbObject*>(net);
     return getDbNwkObjectId(obj);
   }
+  assert(dnet != nullptr);
   return dnet->getId();
 }
 

src/odb/src/db/dbModNet.cpp

@@ -166,6 +166,10 @@ const char* dbModNet::getName() const
 void dbModNet::rename(const char* new_name)
 {
   _dbModNet* obj = (_dbModNet*) this;
+  if (strcmp(obj->_name, new_name) == 0) {
+    return;
+  }
+
   _dbBlock* block = (_dbBlock*) obj->getOwner();
   _dbModule* parent = block->_module_tbl->getPtr(obj->_parent);
   parent->_modnet_hash.erase(obj->_name);

src/rsz/src/RepairHold.cc

@@ -680,140 +680,78 @@ void RepairHold::makeHoldDelay(Vertex* drvr,
                                const Point& loc)
 {
   Pin* drvr_pin = drvr->pin();
-  odb::dbModNet* mod_drvr_net = nullptr;  // hierarchical driver, default none
-  dbNet* db_drvr_net = nullptr;           // regular flat driver
-
-  Instance* parent = nullptr;
-  if (db_network_->hasHierarchy()) {
-    // get the nets on the driver pin (possibly both flat and hierarchical)
-    db_network_->net(drvr_pin, db_drvr_net, mod_drvr_net);
-    // Get the parent instance (owning the instance of the driver pin)
-    // we will put the new buffer in that parent
-    parent = db_network_->getOwningInstanceParent(drvr_pin);
-    // exception case: drvr pin is a top level, fix the db_drvr_net to be
-    // the lower level net. Explictly get the "flat" net.
-    if (network_->isTopLevelPort(drvr_pin)) {
-      db_drvr_net = db_network_->flatNet(db_network_->term(drvr_pin));
-    }
-  } else {
-    // original flat code (which handles exception case at top level &
-    // defaults to top level instance as parent).
-    db_drvr_net = db_network_->staToDb(
-        network_->isTopLevelPort(drvr_pin)
-            ? db_network_->net(db_network_->term(drvr_pin))
-            : db_network_->net(drvr_pin));
-    parent = db_network_->topInstance();
-  }
-  Net *in_net = nullptr, *out_net = nullptr;
-
-  if (loads_have_out_port) {
-    // Verilog uses nets as ports, so the net connected to an output port has
-    // to be preserved.
-    // Move the driver pin over to gensym'd net.
+  dbNet* db_drvr_net = db_network_->findFlatDbNet(drvr_pin);
+  odb::dbModNet* mod_drvr_net = db_network_->hierNet(drvr_pin);
+  Instance* parent = db_network_->getOwningInstanceParent(drvr_pin);
+
+  // If output port is in load pins, do "Driver pin buffering".
+  // - Verilog uses nets as ports, so the net connected to an output port has
+  // - to be preserved.
+  // - Move the driver pin over to gensym'd net.
+  // Otherwise, do "Load pin buffering".
+  bool driver_pin_buffering = loads_have_out_port;
+  Net* buf_input_net = nullptr;
+  Net* buf_output_net = nullptr;
+  if (driver_pin_buffering) {
+    // Do "Driver pin buffering": Buffer drives the existing net.
     //
-    in_net = db_network_->makeNet();
+    //     driver --- (new_net) --- new_buffer ---- (existing_net) ---- loads
+    //
+    buf_input_net = db_network_->makeNet(parent);  // New net
     Port* drvr_port = network_->port(drvr_pin);
     Instance* drvr_inst = network_->instance(drvr_pin);
     sta_->disconnectPin(drvr_pin);
-    sta_->connectPin(drvr_inst, drvr_port, in_net);
-    out_net = db_network_->dbToSta(db_drvr_net);
+    sta_->connectPin(drvr_inst, drvr_port, buf_input_net);
+    buf_output_net = db_network_->dbToSta(db_drvr_net);
   } else {
-    in_net = db_network_->dbToSta(db_drvr_net);
-    // make the output net, put in same module as buffer
-    out_net = db_network_->makeNet(parent);
-  }
-
-  dbNet* in_net_db = db_network_->staToDb(in_net);
-
-  // Disconnect the original drvr pin from everything (hierarchical nets
-  // and flat nets).
-  odb::dbITerm* drvr_pin_iterm;
-  odb::dbBTerm* drvr_pin_bterm;
-  odb::dbModITerm* drvr_pin_moditerm;
-  db_network_->staToDb(
-      drvr_pin, drvr_pin_iterm, drvr_pin_bterm, drvr_pin_moditerm);
-  if (drvr_pin_iterm) {
-    // disconnect the iterm from both the modnet and the dbnet
-    // note we will rewire the drvr_pin to connect to the new buffer later.
-    drvr_pin_iterm->disconnect();
-    drvr_pin_iterm->connect(in_net_db);
-  }
-  if (drvr_pin_moditerm) {
-    drvr_pin_moditerm->disconnect();
+    // Do "Load pin buffering": The existing net drives the new buffer.
+    //
+    //     driver --- (existing_net) --- new_buffer ---- (new_net) ---- loads
+    //
+    buf_input_net = db_network_->dbToSta(db_drvr_net);
+    buf_output_net = db_network_->makeNet(parent);  // New net
   }
 
-  LibertyPort *input, *output;
-  buffer_cell->bufferPorts(input, output);
-
-  // drvr_pin->drvr_net->hold_buffer->net2->load_pins
-
-  // make the buffer in the driver pin's parent
+  // Make the buffer in the driver pin's parent hierarchy
   Instance* buffer = resizer_->makeBuffer(buffer_cell, "hold", parent, loc);
   inserted_buffer_count_++;
   debugPrint(
       logger_, RSZ, "repair_hold", 3, " insert {}", network_->name(buffer));
 
-  // wire in the buffer
-  sta_->connectPin(buffer, input, in_net);
-  sta_->connectPin(buffer, output, out_net);
-
-  // Now patch in the output of the new buffer to the original hierarchical
-  // net,if any, from the original driver
-  if (mod_drvr_net != nullptr) {
-    Pin* ip_pin = nullptr;
-    Pin* op_pin = nullptr;
-    resizer_->getBufferPins(buffer, ip_pin, op_pin);
-    (void) ip_pin;
-    if (op_pin) {
-      // get the iterm of the op_pin of the buffer (a dbInst)
-      // and connect to the hierarchical net.
-      odb::dbITerm* iterm;
-      odb::dbBTerm* bterm;
-      odb::dbModITerm* moditerm;
-      db_network_->staToDb(op_pin, iterm, bterm, moditerm);
-      // we only need to look at the iterm, the buffer is a dbInst
-      if (iterm) {
-        // hook up the hierarchical net
-        iterm->connect(mod_drvr_net);
-      }
-    }
+  LibertyPort *input, *output;
+  buffer_cell->bufferPorts(input, output);
+  Pin* buffer_in_pin = network_->findPin(buffer, input);
+  Pin* buffer_out_pin = network_->findPin(buffer, output);
+
+  // Connect input and output of the new buffer
+  sta_->connectPin(buffer, input, buf_input_net);
+  sta_->connectPin(buffer, output, buf_output_net);
+
+  // TODO: Revisit this. Looks not good.
+  if (mod_drvr_net) {
+    //  The input of the buffer is a new load on the original hierarchical net.
+    db_network_->connectPin(buffer_in_pin, (Net*) mod_drvr_net);
   }
 
-  // hook up loads to buffer
-  for (const Pin* load_pin : load_pins) {
-    if (resizer_->dontTouch(load_pin)) {
-      continue;
-    }
-    dbNet* db_load_net = network_->isTopLevelPort(load_pin)
-                             ? db_network_->flatNet(network_->term(load_pin))
-                             : db_network_->flatNet(load_pin);
-    Net* load_net = db_network_->dbToSta(db_load_net);
-
-    if (load_net != out_net) {
-      Instance* load = db_network_->instance(load_pin);
-      Port* load_port = db_network_->port(load_pin);
-      // record the original connections
-      odb::dbModNet* original_mod_net = nullptr;
-      odb::dbNet* original_flat_net = nullptr;
-      db_network_->net(load_pin, original_flat_net, original_mod_net);
-      (void) original_flat_net;
-      // Remove all the connections on load_pin
-      sta_->disconnectPin(const_cast<Pin*>(load_pin));
-      // Connect it to the correct output driver net
-      sta_->connectPin(load, load_port, out_net);
-      // connect the original load  modnet (hierarchical net), if any,
-      // on the iterm of the buffer created.
-      odb::dbITerm* iterm;
-      odb::dbBTerm* bterm;
-      odb::dbModITerm* moditerm;
-      db_network_->staToDb(load_pin, iterm, bterm, moditerm);
-      if (iterm && original_mod_net) {
-        iterm->connect(original_mod_net);
+  // Buffering target load pins.
+  // - No need in driver pin buffering case
+  if (driver_pin_buffering == false) {
+    // Do load pins buffering
+    for (const Pin* load_pin : load_pins) {
+      if (resizer_->dontTouch(load_pin)) {
+        continue;
       }
+
+      // Disconnect the load pin
+      db_network_->disconnectPin(const_cast<Pin*>(load_pin));
+
+      // Connect with the buffer output
+      db_network_->hierarchicalConnect(db_network_->flatPin(buffer_out_pin),
+                                       db_network_->flatPin(load_pin));
     }
   }
 
-  Pin* buffer_out_pin = network_->findPin(buffer, output);
+  // Update RC and delay. Resize if necessary
   Vertex* buffer_out_vertex = graph_->pinDrvrVertex(buffer_out_pin);
   estimate_parasitics_->updateParasitics();
   // Sta::checkMaxSlewCap does not force dcalc update so do it explicitly.

src/rsz/test/repair_hold1_hier_out.vok

@@ -34,24 +34,27 @@ module top (clk,
     .Q(out));
  BUF_X1 u1 (.A(r2q),
     .Z(u1z));
- AND2_X1 u2 (.A1(r1q),
+ AND2_X1 u2 (.A1(net_o),
     .A2(u1z),
     .ZN(u2z));
- hier1 h1 (.in1(in1),
+ hier1 h1 (.net_o(net_o),
+    .in1(in1),
     .i1z(i1z),
     .op(r1q));
 endmodule
-module hier1 (in1,
+module hier1 (net_o,
+    in1,
     i1z,
     op);
+ output net_o;
  input in1;
  input i1z;
  output op;
 
 
- CLKBUF_X1 hold1 (.A(r1q),
-    .Z(op));
+ CLKBUF_X1 hold1 (.A(op),
+    .Z(net_o));
  DFF_X1 r1 (.D(in1),
     .CK(i1z),
-    .Q(r1q));
+    .Q(op));
 endmodule