Latest STA changes 06/29

[dsengupta0628]

Resolved merge conflict in .github/workflows/ci.yml: pinned actions/checkout to v7 (9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0), keeping SHA-pin style over upstream's @v7 tag.

Replaced deprecated LibertyCell::hasSequentials() with isSequential() (identical impl) at 9 call sites across 5 test files: TestPower.cc, TestLibertyClasses.cc, TestLibertyStaCallbacks.cc, TestLibertyStaBasicsB.cc, TestLibertyStaBasics.cc. Clears all -Wdeprecated-declarations warnings from these files.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 4 committers have signed the CLA.

✅ dsengupta0628
❌ James Cherry
❌ calewis
❌ dependabot[bot]

---

James Cherry seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[gemini-code-assist]

Code Review

This pull request refactors the Newton-Raphson solver in DmpCeff to use Eigen, optimizes generated clock and latch arrival propagation by postponing latch evaluations, introduces new graph traversal helpers, and ensures input delay reference pin edges survive graph rebuilds. The review feedback identifies several critical issues: a potential stale value bug in DmpCeff when nr_order is not 3, an uninitialized pointer and double-free risk in PathEnumFaninVisitor due to an empty copy constructor, numerical instability in the determinant-based singularity check, potential null pointer dereferences in findDelays and visitFanouts, and a division-by-zero risk in Power::findActivity if the clock period is zero.

[gemini-code-assist]

When nr_order_ is not 3 (e.g., nr_order_ == 2 for DmpOnePole), ceff_ is never updated to the input ceff before newtonRaphson(x) is called. Since DmpOnePole::evalDmpEqns relies on ceff_, this leads to stale or uninitialized values being used during the Newton-Raphson iterations. Setting ceff_ = ceff; in the else branch resolves this issue.

Suggested change

	Eigen::Vector3d x = Eigen::Vector3d::Zero();
	if (nr_order_ == 3)
	x_[DmpParam::ceff] = ceff;
	x[DmpParam::ceff] = ceff;
	Eigen::Vector3d x = Eigen::Vector3d::Zero();
	if (nr_order_ == 3)
	x[DmpParam::ceff] = ceff;
	else
	ceff_ = ceff;

[dsengupta0628]

This is not a behavior change. In the base revision, ceff was likewise assigned only under if (nr_order_ == 3); the refactor preserves that exactly. For the order-2 path (DmpOnePole/DmpZeroC2), ceff_ is not a Newton-Raphson variable - it is a fixed value equal to c1_, set in init() (ceff_ = c1) and re-set in DmpZeroC2::gateDelaySlew() prior to use.

evalDmpEqns therefore reads a valid ceff_. The suggested else ceff_ = ceff; would only re-assign c1_ to itself. No stale/uninitialized read occurs.

[gemini-code-assist]

The custom copy constructor PathEnumFaninVisitor::PathEnumFaninVisitor(const PathEnumFaninVisitor &visitor) is empty and does not call the base class copy constructor. This causes the base class to be default-constructed, leaving pred_ uninitialized or null in the copied visitor, which will cause crashes when copy() is called. Additionally, manually deleting pred_ in the destructor will cause a double-free if the base class copy constructor is called and copies the pointer. To fix this robustly, consider passing true for the delete_pred parameter in the PathVisitor constructor so PathVisitor manages the lifetime of pred_, removing the custom destructor entirely, and removing the custom copy constructor entirely to let the compiler generate a correct member-wise copy constructor.

[dsengupta0628]

You are hallucinating. The copy constructor is not empty - it is a delegating constructor that forwards to the primary constructor, which executes PathVisitor(new EnumPred(path_enum), false, path_enum) and allocates a fresh EnumPred. Each copied visitor therefore owns its own distinct pred_.

There is no double-free: PathVisitor::~PathVisitor() deletes only tag_cache_, never pred_. pred_ is deleted exactly once, in ~PathEnumFaninVisitor(), per object.

Also note PathVisitor's constructor has no delete_pred parameter, so that part of the suggestion isn't applicable. The current ownership is correct.

[gemini-code-assist]

The absolute determinant check std::abs(det) < 1e-12 is highly sensitive to the scaling of the matrix elements, which depend on physical units (e.g., Farads and seconds). If the units result in small values in fjac, the determinant can easily fall below 1e-12 even for a well-conditioned matrix, causing unexpected DmpError failures. Using a robust LU decomposition with partial pivoting (as documented in your comments) is much safer and numerically stable.

Suggested change

	if (nr_order_ == 2) {
	double det = fjac.topLeftCorner<2, 2>().determinant();
	if (std::abs(det) < 1e-12) {
	throw DmpError("Jacobian is singular (order 2)");
	}
	p.head<2>() = fjac.topLeftCorner<2, 2>().inverse() * -fvec.head<2>();
	return p;
	}
	}
	// Solves fjac_ * x = p_ for x, assuming fjac_ is LU form from luDecomp.
	// Solution overwrites p_.
	void
	DmpAlg::luSolve()
	{
	const int size = nr_order_;
	// Transform p_ allowing for leading zeros.
	int non_zero = -1;
	for (int i = 0; i < size; i++) {
	int iperm = index_[i];
	double sum = p_[iperm];
	p_[iperm] = p_[i];
	if (non_zero != -1) {
	for (int j = non_zero; j <= i - 1; j++)
	sum -= fjac_[i][j] * p_[j];
	}
	else {
	if (sum != 0.0)
	non_zero = i;
	}
	p_[i] = sum;
	}
	// Backsubstitution.
	for (int i = size - 1; i >= 0; i--) {
	double sum = p_[i];
	for (int j = i + 1; j < size; j++)
	sum -= fjac_[i][j] * p_[j];
	p_[i] = sum / fjac_[i][i];
	double det = fjac.determinant();
	if (std::abs(det) < 1e-12) {
	throw DmpError("Jacobian is singular (order 3)");
	}
	p = fjac.inverse() * -fvec;
	return p;
	}
	if (nr_order_ == 2) {
	auto lu = fjac.topLeftCorner<2, 2>().partialPivLu();
	if (std::abs(lu.matrixLU().diagonal().prod()) < 1e-12) {
	throw DmpError("Jacobian is singular (order 2)");
	}
	p.head<2>() = lu.solve(-fvec.head<2>());
	return p;
	}
	auto lu = fjac.partialPivLu();
	if (std::abs(lu.matrixLU().diagonal().prod()) < 1e-12) {
	throw DmpError("Jacobian is singular (order 3)");
	}
	p = lu.solve(-fvec);
	return p;

[dsengupta0628]

Thanks - you're right that std::abs(det) < 1e-12 is an absolute threshold and not scale-invariant, so it could in principle throw a spurious "singular" error on a well-conditioned but small-magnitude Jacobian. Note the documented LU fallback doesn't actually resolve this: lu.matrixLU().diagonal().prod() equals +/-det, so it uses the same threshold and has the same scale sensitivity - it only improves solve accuracy via pivoting. A real fix would need a relative test (normalize by matrix norm, or fullPivLu().isInvertible()). Will check with James before making any change here

[dsengupta0628]

After discussing with the OpenSTA developer: this isn't a real issue. OpenSTA always works in fixed internal units (seconds), so the Jacobian elements don't rescale with unit choice. So the guard acts as a genuine singularity check and won't false-positive on well-conditioned matrices. So not an issue now

[gemini-code-assist]

Removing the if (arc_delay_calc_) guard in findDelays could lead to null pointer dereferences or unnecessary calculations if arc_delay_calc_ is null. It is safer to keep the guard at the beginning of the function.

  if (!arc_delay_calc_)
    return;
  Stats stats(debug_, report_);

[dsengupta0628]

This guard removal comes from upstream OpenSTA (it is already absent in the base revision of this PR; the diff is against upstream master). arc_delay_calc_ is established at Sta construction and is non-null whenever findDelays() runs. We're intentionally staying aligned with upstream here rather than re-adding a guard that cannot trigger.

[gemini-code-assist]

The pred pointer is dereferenced directly without checking if it is null. To prevent potential null pointer dereferences, consider adding a null check or an assertion.

void
Graph::visitFanouts(Vertex *vertex,
                    SearchPred *pred,
                    const VertexFn &fn)
{
  if (pred && pred->searchFrom(vertex)) {

[dsengupta0628]

pred is non-null by contract, and the sibling functions in this file (visitFanoutEdges, visitFanins, visitFaninEdges) all dereference it the same way. The sole caller (Genclks.cc) passes the address of a stack-local predicate. Adding a null check to only this one function would be inconsistent with the established pattern and guards a case that does not occur.

[gemini-code-assist]

If clk->period() is 0.0, the expression 2.0 / clk->period() will result in a division by zero (producing infinity or NaN). It is safer to guard against a zero period.

    if (clk && clk->period() != 0.0) {
      float duty = clockDuty(clk);
      return PwrActivity(2.0 / clk->period(), duty, PwrActivityOrigin::clock);
    }

[dsengupta0628]

A clock with a zero period is degenerate - create_clock enforces a positive period - so clk->period() is never 0.0 here, and the other period() divisions in this file (e.g. clockDuty) are likewise unguarded. The null-clk guard added in this PR addresses the real defect. A zero-period check would be redundant.

[dsengupta0628]

CI fails because we need OpenROAD side changes to accomodate these:

The-OpenROAD-Project/OpenROAD#10790