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;

[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.

[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;

[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_);

[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)) {

[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]

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

The-OpenROAD-Project/OpenROAD#10790