Skip to content

syn: Add gate remapping by Ashenhurst-Curtis decomposition with shared set#10906

Open
povik wants to merge 10 commits into
The-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:syn-acd
Open

syn: Add gate remapping by Ashenhurst-Curtis decomposition with shared set#10906
povik wants to merge 10 commits into
The-OpenROAD-Project:masterfrom
The-OpenROAD-Project-staging:syn-acd

Conversation

@povik

@povik povik commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

[Describe your changes here]

Type of Change

  • New feature

Impact

New syn::acd_resynth command for invoking timing-oriented ACD resynthesis. This has the form of an undocumented developer command in the interim before we get SDC propagation infrastructure which will allow us to fold this optimization step directly into synthesize.

Verification

  • I have verified that the local build succeeds (./etc/Build.sh).
  • I have run the relevant tests and they pass.
  • My code follows the repository's formatting guidelines.
  • I have included tests to prevent regressions.
  • I have signed my commits (DCO).

Related Issues

[Link issues here]

povik added 2 commits July 14, 2026 01:31
Signed-off-by: Martin Povišer <povik@cutebit.org>
Assisted-by: Claude (various models)
Signed-off-by: Martin Povišer <povik@cutebit.org>
@povik povik self-assigned this Jul 13, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the Ashenhurst-Curtis decomposition (ACD) flow for gate-level resynthesis, adding new files for ACD synthesis, timing, and target indexing, while refactoring the combinational mapper to use the new target index. The review feedback highlights several critical issues and improvements: potential out-of-bounds memory accesses in ArrivalSet::atExit (when outputs exceed two) and outputArrival (when a port is not found), an unconditional overwrite of the budget parameter that ignores caller-specified limits, a redundant null check for sta in populateCutTiming, and a missing continue statement in buildIndex when handling tie cells with zero inputs.

Comment thread src/syn/src/flow/acd.cc
Comment thread src/syn/src/flow/acd_timing.cc
Comment thread src/syn/src/flow/acd.cc Outdated
Comment thread src/syn/src/flow/acd_timing.cc Outdated
Comment on lines +191 to +193
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When inputs.empty() is true, tie cells are identified and stored in index.tie_low / index.tie_high. However, the loop does not continue afterward, meaning the code will proceed to evaluate fexprEval and npnSemiclassAllRepr with 0 inputs, which is buggy and redundant. Adding a continue avoids this.

Suggested change
}
}
}
index.tie_high = {cell, 0};
}
}
continue;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not resolved?

povik added 8 commits July 16, 2026 13:55
Assisted-by: Claude (various models)
Signed-off-by: Martin Povišer <povik@cutebit.org>
Assisted-by: Claude (various models)
Signed-off-by: Martin Povišer <povik@cutebit.org>
Assisted-by: Claude (various models)
Signed-off-by: Martin Povišer <povik@cutebit.org>
Signed-off-by: Martin Povišer <povik@cutebit.org>
Signed-off-by: Martin Povišer <povik@cutebit.org>
Signed-off-by: Martin Povišer <povik@cutebit.org>
Assisted-by: Claude (various models)
Signed-off-by: Martin Povišer <povik@cutebit.org>
Assisted-by: Claude (various models)
Signed-off-by: Martin Povišer <povik@cutebit.org>
@povik
povik marked this pull request as ready for review July 18, 2026 20:47
@povik
povik requested a review from a team as a code owner July 18, 2026 20:47
@povik
povik requested a review from maliberty July 18, 2026 20:47
@maliberty

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 138ac4b393

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

const float cand_slack
= networkSlack(logger_, problem, cand, corner_, fixed_slew_);

if (cand_slack <= slack + epsilon_) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Require per-output slack before accepting cuts

For a two-output cut, this accepts a replacement solely because the cut-wide worst slack improves. If the original roots have slacks like -100ps and -20ps, a candidate with -50ps and -80ps passes this check because the minimum improved from -100ps to -80ps, even though the second endpoint lost 60ps; the same can turn a previously positive side output negative. Since commit() rewires all roots of the substitution, the acceptance test should also enforce per-root no-regression/non-violation, not just WNS improvement.

Useful? React with 👍 / 👎.

Comment on lines +407 to +410
odb::dbInst* inst
= odb::dbInst::create(block,
db_network_->staToDb(cell),
("remap" + std::to_string(made_++)).c_str());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use unique remap instance names

When the input design already contains an instance named remap0/remapN (or a netlist from a previous run is reloaded), this raw dbInst::create returns nullptr on duplicate names in ODB, and the following inst->getITerm(...) dereferences it. Use ODB's uniquifying create/helper for generated remap objects so acd_resynth -apply cannot crash on ordinary name collisions.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants