odb: encapsulate inst module assignment

src/odb/src/db/dbBlock.cpp

@@ -871,7 +871,7 @@ dbOStream& operator<<(dbOStream& stream, const _dbBlock& block)
  * @param block The database block containing the objects.
  * @param table The internal database table storing the object implementations.
  * @param module_field Member pointer to the field storing the parent module ID
- *                     (e.g., &_dbInst::module_ or &_dbModInst::parent_).
+ *                     (e.g., &_dbModInst::parent_).
  * @param hash_field Member pointer to the hash map member in _dbModule where
  *                   the object ID should be stored.
  */
@@ -896,6 +896,22 @@ static void rebuildModuleHash(
   }
 }
 
+static void rebuildDbInstModuleHash(_dbBlock& block)
+{
+  dbSet<dbInst> items((dbBlock*) &block, block.inst_tbl_);
+  for (dbInst* obj : items) {
+    _dbInst* _obj = (_dbInst*) obj;
+    dbId<_dbModule> mid = _obj->getModuleId();
+    if (mid == 0) {
+      mid = block.top_module_;
+    }
+    _dbModule* module = block.module_tbl_->getPtr(mid);
+    if (module && _obj->name_) {
+      module->dbinst_hash_[_obj->name_] = _obj->getId();
+    }
+  }
+}
+
 dbIStream& operator>>(dbIStream& stream, _dbBlock& block)
 {
   _dbDatabase* db = block.getImpl()->getDatabase();
@@ -989,8 +1005,7 @@ dbIStream& operator>>(dbIStream& stream, _dbBlock& block)
   }
   if (db->isSchema(kSchemaDbRemoveHash)) {
     // Construct dbinst_hash_
-    rebuildModuleHash<dbInst>(
-        block, block.inst_tbl_, &_dbInst::module_, &_dbModule::dbinst_hash_);
+    rebuildDbInstModuleHash(block);
   }
   if (db->isSchema(kSchemaBlockOwnsScanInsts)) {
     stream >> *block.scan_inst_tbl_;

src/odb/src/db/dbInst.cpp

@@ -87,6 +87,19 @@ void _dbInst::setInstBBox(_dbInst* inst)
   block->add_rect(box->shape_.rect);
 }
 
+void _dbInst::setModule(dbId<_dbModule> module)
+{
+  if (module_ == module) {
+    return;
+  }
+
+  module_ = module;
+  _dbBlock* block = (_dbBlock*) getOwner();
+  for (dbBlockCallBackObj* cb : block->callbacks_) {
+    cb->inDbPostInstParentChange((dbInst*) this);
+  }
+}
+
 _dbInst::_dbInst(_dbDatabase*)
 {
   flags_.orient = dbOrientType::R0;
@@ -157,7 +170,7 @@ dbOStream& operator<<(dbOStream& stream, const _dbInst& inst)
   stream << inst.inst_hdr_;
   stream << inst.bbox_;
   stream << inst.region_;
-  stream << inst.module_;
+  stream << inst.getModuleId();
   stream << inst.group_;
   stream << inst.region_next_;
   stream << inst.module_next_;
@@ -185,7 +198,9 @@ dbIStream& operator>>(dbIStream& stream, _dbInst& inst)
   stream >> inst.inst_hdr_;
   stream >> inst.bbox_;
   stream >> inst.region_;
-  stream >> inst.module_;
+  dbId<_dbModule> module;
+  stream >> module;
+  inst.setModuleQuiet(module);
   stream >> inst.group_;
   stream >> inst.region_next_;
   stream >> inst.module_next_;
@@ -866,12 +881,13 @@ dbModule* dbInst::getModule()
 {
   _dbInst* inst = (_dbInst*) this;
 
-  if (inst->module_ == 0) {
+  const dbId<_dbModule> module_id = inst->getModuleId();
+  if (module_id == 0) {
     return nullptr;
   }
 
   _dbBlock* block = (_dbBlock*) inst->getOwner();
-  _dbModule* module = block->module_tbl_->getPtr(inst->module_);
+  _dbModule* module = block->module_tbl_->getPtr(module_id);
   return (dbModule*) module;
 }
 
@@ -1072,7 +1088,7 @@ bool dbInst::isPhysicalOnly()
 {
   _dbInst* inst = (_dbInst*) this;
 
-  return inst->module_ == 0;
+  return inst->getModuleId() == 0;
 }
 
 dbInst* dbInst::getParent()
@@ -1589,7 +1605,7 @@ void dbInst::destroy(dbInst* inst_)
     block->journal_->pushParam(inst->x_);
     block->journal_->pushParam(inst->y_);
     block->journal_->pushParam(inst->group_);
-    block->journal_->pushParam(inst->module_);
+    block->journal_->pushParam(inst->getModuleId());
     block->journal_->pushParam(inst->region_);
     block->journal_->endAction();
   }

src/odb/src/db/dbInst.h

@@ -24,6 +24,7 @@ class _dbGroup;
 class _dbChipBump;
 class _dbChipRegion;
 class dbInst;
+class dbModule;
 class dbIStream;
 class dbOStream;
 
@@ -63,6 +64,9 @@ class _dbInst : public _dbObject
   void collectMemInfo(MemInfo& info);
   static void setInstBBox(_dbInst* inst);
 
+  dbId<_dbModule> getModuleId() const { return module_; }
+  void setModule(dbId<_dbModule> module);
+
   _dbInstFlags flags_;
   char* name_;
   int x_;
@@ -72,14 +76,16 @@ class _dbInst : public _dbObject
   dbId<_dbInstHdr> inst_hdr_;
   dbId<_dbBox> bbox_;
   dbId<_dbRegion> region_;
-  // WARNING: re-parenting an existing instance changes its full SDC
-  // path. dbModule::addInst() fires inDbPostInstParentChange in that
-  // case so downstream caches (e.g., dbSdcNetwork's path-to-instance
-  // map) stay consistent. The callback is suppressed on the initial
-  // assignment during dbInst::create -- that path's accounting belongs
-  // to inDbInstCreate. Prefer dbModule::addInst() over assigning this
-  // field directly.
+
+ private:
+  friend class dbModule;
+  friend class _dbModule;
+  friend dbIStream& operator>>(dbIStream& stream, _dbInst& inst);
+
   dbId<_dbModule> module_;
+  void setModuleQuiet(dbId<_dbModule> module) { module_ = module; }
+
+ public:
   dbId<_dbGroup> group_;
   dbId<_dbInst> region_next_;
   dbId<_dbInst> module_next_;

src/odb/src/db/dbModule.cpp

@@ -224,7 +224,8 @@ void dbModule::addInst(dbInst* inst)
         getName());
   }
 
-  if (_inst->module_ == module->getOID()) {
+  const dbId<_dbModule> old_module_id = _inst->getModuleId();
+  if (old_module_id == module->getOID()) {
     return;  // already in this module
   }
 
@@ -240,13 +241,12 @@ void dbModule::addInst(dbInst* inst)
   // initial assignment during dbInst::create (module_ is unset). Only
   // the former should fire inDbPostInstParentChange -- otherwise every
   // create would falsely trigger downstream subtree-invalidation paths.
-  const bool is_reparent = (_inst->module_ != 0);
+  const bool is_reparent = (old_module_id != 0);
   if (is_reparent) {
-    dbModule* mod = dbModule::getModule((dbBlock*) block, _inst->module_);
+    dbModule* mod = dbModule::getModule((dbBlock*) block, old_module_id);
     ((_dbModule*) mod)->removeInst(inst);
   }
 
-  _inst->module_ = module->getOID();
   module->dbinst_hash_[inst->getName()] = dbId<_dbInst>(_inst->getOID());
 
   if (module->insts_ == 0) {
@@ -261,9 +261,10 @@ void dbModule::addInst(dbInst* inst)
   }
 
   if (is_reparent) {
-    for (dbBlockCallBackObj* cb : block->callbacks_) {
-      cb->inDbPostInstParentChange(inst);
-    }
+    _inst->setModule(module->getOID());
+  } else {
+    // Initial dbInst::create ownership is reported through inDbInstCreate.
+    _inst->setModuleQuiet(module->getOID());
   }
 }
 
@@ -273,7 +274,7 @@ void _dbModule::removeInst(dbInst* inst)
   _dbInst* _inst = (_dbInst*) inst;
   uint32_t id = _inst->getOID();
 
-  if (_inst->module_ != getOID()) {
+  if (_inst->getModuleId() != getOID()) {
     return;
   }
 
@@ -305,7 +306,7 @@ void _dbModule::removeInst(dbInst* inst)
       prev->module_next_ = _inst->module_next_;
     }
   }
-  _inst->module_ = 0;
+  _inst->setModuleQuiet(0);
   _inst->module_next_ = 0;
   _inst->module_prev_ = 0;
 }

src/odb/test/cpp/CallBack.h

@@ -38,6 +38,13 @@ class CallBack : public dbBlockCallBackObj
       events.push_back("Destroy inst " + inst->getName());
     }
   }
+  void inDbPostInstParentChange(dbInst* inst) override
+  {
+    if (!_pause) {
+      events.push_back("Change parent of inst " + inst->getName() + " to "
+                       + inst->getModule()->getName());
+    }
+  }
   void inDbInstSwapMasterBefore(dbInst* inst, dbMaster* master) override
   {
     if (!_pause) {

src/odb/test/cpp/TestCallBacks.cpp

@@ -86,6 +86,38 @@ TEST_F(CallbackFixture, test_inst_and_iterm)
   EXPECT_EQ(cb_.events[3], "Destroy inst i1");
 }
 
+TEST_F(CallbackFixture, test_inst_parent_change)
+{
+  createSimpleDB();
+  dbBlock* block = db_->getChip()->getBlock();
+  cb_.addOwner(block);
+
+  dbInst* inst = dbInst::create(block, db_->findMaster("and2"), "i1");
+  EXPECT_EQ(cb_.events.size(), 4);
+  cb_.clearEvents();
+
+  dbModule* parent = dbModule::create(block, "parent");
+  parent->addInst(inst);
+  ASSERT_EQ(cb_.events.size(), 1);
+  EXPECT_EQ(cb_.events[0], "Change parent of inst i1 to parent");
+  cb_.clearEvents();
+
+  parent->addInst(inst);
+  EXPECT_TRUE(cb_.events.empty());
+
+  dbModule* top = block->getTopModule();
+  top->addInst(inst);
+  ASSERT_EQ(cb_.events.size(), 1);
+  EXPECT_EQ(cb_.events[0],
+            "Change parent of inst i1 to " + std::string(top->getName()));
+  cb_.clearEvents();
+
+  top->addInst(inst);
+  EXPECT_TRUE(cb_.events.empty());
+
+  dbInst::destroy(inst);
+}
+
 TEST_F(CallbackFixture, test_net)
 {
   createSimpleDB();