odb: Refactored insert buffer code #9182

openroad-ci · 2026-01-01T07:16:12Z

Added const qualifiers.
Added std::vector.reserve(16) calls.
Minor name changes.
Fixed lint issues.

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

github-actions · 2026-01-01T07:21:09Z

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

QuantamHD · 2026-01-01T07:21:17Z

src/odb/src/db/dbModNet.cpp

-  std::vector<dbModNet*> worklist;
  std::set<dbModNet*> visited;
+  std::vector<dbModNet*> worklist;
+  worklist.reserve(16);


Is the idea here that most often we need just a small number of list items? If so using boost small vector would be a better choice. The current code will always cause a heap allocation, boost small vector will allocate up to N items on the stack, and only go to the heap if the initial capacity is exceeded.

https://www.boost.org/doc/libs/1_69_0/doc/html/boost/container/small_vector.html

small_vector looks good. Thanks for the suggestion!

gemini-code-assist

Code Review

This pull request primarily focuses on improving const-correctness and clarity across several OpenDB (ODB) classes, particularly dbNet, dbModule, and dbInsertBuffer. Key changes include adding const qualifiers to numerous method parameters and return types, such as load_pins in buffer insertion functions and various get and find methods in dbModule and dbInsertBuffer. Function parameter names were also updated for better readability, like changing mcf to MillerMult in dbNet::getGndTotalCap and dbid_ to oid in dbNet::getNet functions. Additionally, internal helper functions for buffer placement were renamed for more descriptive purposes (e.g., placeNewBuffer to placeBufferAtLocationOrPin). Performance improvements were made in dbModNet by pre-allocating vector capacity. The documentation for buffer insertion APIs was updated to reflect default parameter values. Review comments highlighted and suggested fixes for several instances of const_cast and potential const-correctness violations, specifically in dbInsertBuffer::findLCA and dbModule::getOwner, recommending changes to return const pointers. The reviewer also suggested reverting some parameter name changes, such as rid back to rseg_id in dbNet::set1stRSegId and mapFile back to capNodeMap in dbNet::collapseInternalCapNum, for improved descriptiveness.

gemini-code-assist · 2026-01-01T07:24:02Z

src/odb/src/db/dbInsertBuffer.cpp

    m2 = m2->getParentModule();
  }
-  return m1;
+  return const_cast<dbModule*>(m1);


Using const_cast here breaks const-correctness. The function findLCA is declared const and takes const pointers, but it returns a non-const pointer. This can lead to unintended modifications of what should be a const object.

To maintain const-correctness, it's better to change the return type of findLCA to const dbModule*. This may require propagating const to the calling functions, but it leads to safer and more robust code. I've added a suggestion on the header file to fix this.

However, the suggested solution makes using this API more difficult.

I'm confused by the original comment saying "findLCA is declared const and takes const pointers" as it signature was

findLCA(dbModule* m1, dbModule* m2) const

which doesn't take const pointers. I see no const_cast at all in the original. I think this is a bad suggestion and would undo the change it suggested.

gemini-code-assist · 2026-01-01T07:24:02Z

src/odb/src/db/dbInsertBuffer.h

-                          dbITerm* drvr_term,
-                          const std::set<dbObject*>& load_pins);
+  int getModuleDepth(const dbModule* mod) const;
+  dbModule* findLCA(const dbModule* m1, const dbModule* m2) const;


This function is const and takes const pointers, but returns a non-const pointer. This breaks const-correctness and requires a const_cast in the implementation. To fix this, the return type should be const dbModule*. This will likely require propagating const to callers of findLCA.

Suggested change

dbModule* findLCA(const dbModule* m1, const dbModule* m2) const;

const dbModule* findLCA(const dbModule* m1, const dbModule* m2) const;

again not true " takes const pointers,"

gemini-code-assist · 2026-01-01T07:24:02Z

src/odb/src/db/dbModule.cpp

+dbBlock* dbModule::getOwner() const
 {
  _dbModule* obj = (_dbModule*) this;
  return (dbBlock*) obj->getOwner();


The cast (_dbModule*) this inside a const member function is a const_cast in disguise, which breaks const-correctness. This function should return a const dbBlock* to be const-correct.

Consider changing the function signature in db.h and this implementation to:
const dbBlock* dbModule::getOwner() const
{
const _dbModule* obj = (const _dbModule*) this;
return (const dbBlock*) obj->getOwner();
}
This would require _dbObject::getOwner() to be a const method, and callers of dbModule::getOwner() might need to be updated to handle a const dbBlock*.

const dbBlock* dbModule::getOwner() const { const _dbModule* obj = (const _dbModule*) this; return (const dbBlock*) obj->getOwner(); }

I wouldn't change the return type but you could do

const _dbModule* obj = (const _dbModule*) this;

gemini-code-assist · 2026-01-01T07:24:02Z

src/odb/include/odb/db.h

  /// Set the 1st R segment of this net.
  ///
-  void set1stRSegId(uint32_t rseg_id);
+  void set1stRSegId(uint32_t rid);


The parameter name rid is less descriptive than the original rseg_id. While the function name set1stRSegId provides context, a more descriptive name like rseg_id or rsegId would improve readability and clarity, especially for developers unfamiliar with this part of the codebase.

Suggested change

void set1stRSegId(uint32_t rid);

void set1stRSegId(uint32_t rseg_id);

gemini-code-assist · 2026-01-01T07:24:02Z

src/odb/include/odb/db.h

  /// compact internal capnode number'
  ///
-  void collapseInternalCapNum(FILE* capNodeMap);
+  void collapseInternalCapNum(FILE* mapFile);


The new parameter name mapFile is more generic than the original capNodeMap. Given the function collapseInternalCapNum and its comment "compact internal capnode number", capNodeMap seems more descriptive of the file's contents. Consider reverting to the original name for better clarity.

Suggested change

void collapseInternalCapNum(FILE* mapFile);

void collapseInternalCapNum(FILE* capNodeMap);

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

github-actions · 2026-01-01T10:21:39Z

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

maliberty · 2026-01-01T16:12:10Z

src/odb/src/db/dbInsertBuffer.cpp

    m2 = m2->getParentModule();
  }
-  return m1;
+  return const_cast<dbModule*>(m1);


I'm confused by the original comment saying "findLCA is declared const and takes const pointers" as it signature was

findLCA(dbModule* m1, dbModule* m2) const

which doesn't take const pointers. I see no const_cast at all in the original. I think this is a bad suggestion and would undo the change it suggested.

maliberty · 2026-01-01T16:16:13Z

src/odb/src/db/dbInsertBuffer.h

-                          dbITerm* drvr_term,
-                          const std::set<dbObject*>& load_pins);
+  int getModuleDepth(const dbModule* mod) const;
+  dbModule* findLCA(const dbModule* m1, const dbModule* m2) const;


again not true " takes const pointers,"

maliberty · 2026-01-01T16:19:47Z

src/odb/src/db/dbModule.cpp

+dbBlock* dbModule::getOwner() const
 {
  _dbModule* obj = (_dbModule*) this;
  return (dbBlock*) obj->getOwner();


I wouldn't change the return type but you could do

const _dbModule* obj = (const _dbModule*) this;

maliberty · 2026-01-01T16:20:49Z

src/odb/src/db/dbNet.cpp

 }

-void dbNet::getGndTotalCap(double* gndcap, double* totalcap, double mcf)
+void dbNet::getGndTotalCap(double* gndcap, double* totalcap, double MillerMult)


I don't see any connection to insert buffer. MillerMult should be miller_mult (or miller_coupling_factor to match mcf).

This is not related to insert_buffer. I just fixed lint issues I found by chance.

Let me change it by using millter_mult including other APIs as follows.

miller_coupling_factor is somewhat long.

mcf looks not intuitive.

Other APIs using MillerMult

void addGndTotalCap(double* gndcap, double* totalcap, double MillerMult); void accAllCcCap(double* totalcap, double MillerMult); double getCapacitance(int corner, double MillerMult);

maliberty · 2026-01-01T16:25:03Z

src/odb/src/db/dbInsertBuffer.cpp

-void dbInsertBuffer::placeNewBuffer(dbInst* buffer_inst,
-                                    const Point* loc,
-                                    dbObject* term)
+void dbInsertBuffer::placeBufferAtLocationOrPin(dbInst* buffer_inst,


I think it clearer to have two single-purposes methods (pin / location).

I see the thumbs up but I don't see any change

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

github-actions · 2026-01-02T01:27:40Z

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

jhkim-pii added 3 commits January 1, 2026 15:07

odb: Minor refactoring of insert buffer related codes.

2ffffc6

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

Fixed lint issue

3979bd5

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

odb: Fixed lint issue

b3b0c7f

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

openroad-ci assigned jhkim-pii Jan 1, 2026

QuantamHD reviewed Jan 1, 2026

View reviewed changes

gemini-code-assist bot reviewed Jan 1, 2026

View reviewed changes

odb: Used small_vector. Minor refactoring

795bf22

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

jhkim-pii requested a review from maliberty January 1, 2026 12:46

maliberty requested changes Jan 1, 2026

View reviewed changes

jhkim-pii added 4 commits January 2, 2026 10:06

odb: Modified const qualifiers

eb0b8b7

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

odb: Renamed (MillerMult -> miller_mult)

75e3d3c

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

odb: Fixed null pointer dereference issue

c4e6ce8

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

odb: Fixed clang-format issue

5924a8b

Signed-off-by: Jaehyun Kim <jhkim@precisioninno.com>

jhkim-pii requested a review from maliberty January 2, 2026 04:29

	dbModule* findLCA(const dbModule* m1, const dbModule* m2) const;
	const dbModule* findLCA(const dbModule* m1, const dbModule* m2) const;

	void set1stRSegId(uint32_t rid);
	void set1stRSegId(uint32_t rseg_id);

	void collapseInternalCapNum(FILE* mapFile);
	void collapseInternalCapNum(FILE* capNodeMap);

odb: Refactored insert buffer code #9182

Are you sure you want to change the base?

odb: Refactored insert buffer code #9182

Conversation

openroad-ci commented Jan 1, 2026

Uh oh!

github-actions bot commented Jan 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jan 1, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants