Sta upstream 10/05

[dsengupta0628]

Latest STA from 10/05 morning upstream

[gemini-code-assist]

Code Review

This pull request refactors several core components of the timing analysis engine, including replacing dynamic PortDirection singletons with static constexpr instances, optimizing levelization with lazy initialization, adding port limit calculations to timing models, and improving Tcl error tracebacks. However, several critical issues were identified in the review: a performance bottleneck in findCrpr due to heap allocations and O(N log N) complexity from std::map, a typo in Levelize::findBackEdges that iterates over roots_ instead of the sorted roots sequence (introducing non-determinism), a bug in zeroSlewAndWireDelays where passing true for merge prevents proper zeroing, and variable shadowing of pin in setPortLimits.

[gemini-code-assist]

Using std::map here introduces $O(N \log N)$ complexity and, more importantly, performs heap allocations for every node in the path. Since findCrpr is called frequently during timing analysis, this can be a significant performance bottleneck.\n\nSince the clock path is a tree and prevPath() acts as a parent pointer, finding the common pin is equivalent to finding the Lowest Common Ancestor (LCA) of two nodes in a tree. We can achieve this in $O(N)$ time and $O(1)$ auxiliary space without any heap allocations by using the standard two-pointer technique (aligning the path depths first and then walking in lockstep).

  int src_len = 0;
  const Path *src_curr = src_clk_path1;
  while (src_curr && !src_curr->isNull()) {
    src_len++;
    src_curr = src_curr->prevPath();
  }

  int tgt_len = 0;
  const Path *tgt_curr = tgt_clk_path1;
  while (tgt_curr && !tgt_curr->isNull()) {
    tgt_len++;
    tgt_curr = tgt_curr->prevPath();
  }

  const Path *src = src_clk_path1;
  const Path *tgt = tgt_clk_path1;
  if (src_len > tgt_len) {
    for (int i = 0; i < src_len - tgt_len; ++i) {
      src = src->prevPath();
    }
  } else if (tgt_len > src_len) {
    for (int i = 0; i < tgt_len - src_len; ++i) {
      tgt = tgt->prevPath();
    }
  }

  Pin *tgt_pin = nullptr;
  while (src && !src->isNull() && tgt && !tgt->isNull()) {
    if (src->pin(this) == tgt->pin(this)) {
      tgt_pin = tgt->pin(this);
      break;
    }
    src = src->prevPath();
    tgt = tgt->prevPath();
  }

[gemini-code-assist]

There is a typo here: the code initializes a local roots sequence sorted by fanout to ensure deterministic traversal, but then iterates over the member variable roots_ instead. Since roots_ is a VertexSet (which may be ordered by pointer addresses or unordered), iterating over it directly can introduce non-determinism in the DFS traversal and loop-breaking order.

  VertexSeq roots = sortedRootsWithFanout();
  for (Vertex *vertex : roots) {

[dsengupta0628]

likely typo upstream- needs fixing

[gemini-code-assist]

In zeroSlewAndWireDelays, passing true for the merge parameter to annotateLoadDelaySlew is a bug. The purpose of this function is to unconditionally reset/zero the downstream wire delays and slews (as done in the original code). If merge is true, the values will only be updated if they satisfy the delayGreater condition, which prevents them from being reset to 0.0 if they already have non-zero values. This should be false to match the original unconditional zeroing behavior.

          annotateLoadDelaySlew(drvr_vertex, wire_edge, delay_zero,
                                load_vertex, delay_zero,
                                false, rf, min_max, ap_index);

[gemini-code-assist]

The loop variable pin shadows the function parameter pin. Renaming the loop variable to conn_pin avoids shadowing and improves code clarity.

    const Pin *conn_pin = pin_iter->next();
    const LibertyPort *port = network_->libertyPort(conn_pin);

[maliberty]

Have you checked for QoR impact?

[dsengupta0628]

Have you checked for QoR impact?

not yet- there seems to be a correctness issue that I have raised with upstream. will wait for that before overloading CI with runs