Debug or runtime fix

[openroad-ci]

changes in dbNetwork

[dsengupta0628]

Details in attached ppt
ascenium_design_runtime_debug.pptx

[gemini-code-assist]

Code Review

This pull request introduces a significant performance optimization by caching _dbMTerm pointers in _dbInstHdr, which speeds up getMTerm() lookups. The implementation of this caching mechanism appears to be thorough, with updates to object creation, serialization, and copy logic. Additionally, there's a fix in dbNetwork::libertyPort to correctly resolve liberty ports for pins, and an attempted optimization in getDbNwkObjectId. My review focuses on a potential issue with the optimization in getDbNwkObjectId where a bounds check is bypassed for common object types, which could lead to data corruption. Please see the specific comment for details.

[gemini-code-assist]

The bounds check for db_id has been moved after the early returns for common object types (dbInstObj, dbNetObj, dbITermObj). This means that for these common types, the check is now skipped. If a large db_id is encountered for one of these types, it could lead to a silently corrupted ObjectId due to the left shift operation.

While the comment suggests this is intentional ("Bounds check only for debug/rare cases"), it introduces a risk of data corruption that was prevented by the original code. The performance gain from skipping this single if statement is likely minimal, while the risk is significant.

It is recommended to move the bounds check back to before the type-specific handling to ensure all db_id values are validated.

  if (db_id > (std::numeric_limits<ObjectId>::max() >> DBIDTAG_WIDTH)) {
    logger_->error(ORD, 2019, "Database id exceeds capacity");
  }

  // Optimize for most common types to avoid switch overhead
  if (typ == dbInstObj) {
    return ((db_id << DBIDTAG_WIDTH) | DBINST_ID);
  }
  if (typ == dbNetObj) {
    return ((db_id << DBIDTAG_WIDTH) | DBNET_ID);
  }
  if (typ == dbITermObj) {
    return ((db_id << DBIDTAG_WIDTH) | DBITERM_ID);
  }

[dsengupta0628]

Resolved in new version

[github-actions]

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

[github-actions]

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

[QuantamHD]

Since we're using c++20 you might want to give [[likely]]/[[unlikely]] a try to see if we can preserve the case switch statement to get the compiler errors in case we add a new enum.

https://en.cppreference.com/w/cpp/language/attributes/likely

 switch (typ) {
   [[likely]] case dbITermObj: {
      return ((db_id << DBIDTAG_WIDTH) | DBITERM_ID);
    } break;
    [[unlikely]]case dbBTermObj: {
      return ((db_id << DBIDTAG_WIDTH) | DBBTERM_ID);
    } break;
    case dbInstObj: {
      return ((db_id << DBIDTAG_WIDTH) | DBINST_ID);
    } break;
    [[likely]]case dbNetObj: {
      return ((db_id << DBIDTAG_WIDTH) | DBNET_ID);
    } break;
    case dbModITermObj: {
      return ((db_id << DBIDTAG_WIDTH) | DBMODITERM_ID);
    } break;