dft: implement scan_opt using the 2-Opt algorithm by donn · Pull Request #10176 · The-OpenROAD-Project/OpenROAD

donn · 2026-04-17T19:39:54Z

Summary

dft::Dft:
- isolate writing the data to odb to its own function
- store scan chains for later optimization after stitching
dft::Dft::scanOpt(): use scan chain driver/sink information plus instance placements to optimize the total scan chain length using the 2Opt algorithm
dft::ScanChain:
add estimateInternalTWL() to return an estimated total wire length for the scan chain based on cell origins (for verification/testing purposes)
sortScanCells now returns an estimated total wire length (and requires lambdas passed to it to do so as well)
dft::ScanPin: add getLocation() as a a method to get terminal locations whether it's a bterm or an iterm
odb::dbDft: Add reset() method to clear all existing info

Type of Change

New feature
Refactoring

Impact

Measurable reduction in routing time versus the unoptimized scan chains.

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 signed my commits (DCO).

- dft::Dft: - isolate writing the data to odb to its own function - store scan chains for later optimization after stitching - dft::Dft::scanOpt(): use scan chain driver/sink information plus instance placements to optimize the total scan chain length using the 2Opt algorithm - dft::ScanChain: - add estimateInternalTWL() to return an estimated total wire length for the scan chain based on cell origins (for verification/testing purposes) - sortScanCells now returns an estimated total wire length (and requires lambdas passed to it to do so as well) - dft::ScanPin: add getLocation() as a a method to get terminal locations whether it's a bterm or an iterm - odb::dbDft: Add reset() method to clear all existing info - utl::Logger::error(): replace sprintf with snprintf so clang doesn't report an error on every single use of the function. Signed-off-by: Mohamed Gaber <me@donn.website>

donn · 2026-04-17T19:40:48Z

I know I still need to update the Readme but given the average turnaround on contributing to the dft module is about a month I figured may as well just get started and get feedback first

gemini-code-assist

Code Review

This pull request implements scan chain optimization using a 2-Opt heuristic to reduce total wire length and introduces several architectural improvements to the DFT module, such as the writeToOdb method and state resetting in dbDft. Key feedback includes addressing a critical issue where a side effect inside an assert would be stripped in release builds, potentially causing incorrect behavior. Additionally, a return statement in the optimization loop should be changed to continue to avoid skipping valid chains when one fails, and an initialization of a size_t index to -1 needs to be corrected to prevent out-of-bounds access.

gemini-code-assist · 2026-04-17T19:45:00Z

+  }
+  points[++i] = sink;
+  score += odb::Point::manhattanDistance(last, sink);
+  assert(++i == N);


The assert contains a side effect (++i). In release builds where NDEBUG is defined, the assertion and its expression are removed, meaning i will not be incremented. This will lead to incorrect behavior in release builds. Side effects should be moved outside of the assert.

Suggested change

assert(++i == N);

++i;

assert(i == N);

Well i is not used after this point but fair enough

gemini-code-assist · 2026-04-17T19:45:00Z

+                    14,
+                    "Cannot optimize: source/sink pins don't exist or have no "
+                    "placement.");
+      return;


The return statement here will cause the entire scanOpt function to exit if a single scan chain cannot be optimized (e.g., due to missing source/sink pins or placement). This prevents other valid scan chains from being optimized. It should be replaced with continue to skip only the current chain.

Suggested change

return;

continue;

gemini-code-assist · 2026-04-17T19:45:00Z

+  size_t N = cells.size() + 2;
+
+  std::vector<odb::Point> points(N);
+  size_t i = -1;


In this repository, static_cast<size_t>(-1) is the standard sentinel value for an invalid size_t index because 0 is often a valid index. However, using -1 (which wraps to the maximum value of size_t) as an active index for points[i] will cause an out-of-bounds access. If i is intended to be a valid starting index, it should be initialized to 0.

Suggested change

size_t i = -1;

size_t i = 0;

points[i] = source;

References

When checking for failure or sentinel values in size_t, use static_cast<size_t>(-1) because 0 is a valid index.

Doing this instead seems a bit clearer and achieves the same thing.

size_t i = 0; points[i++] = source;

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 28. Check the log or trigger a new build to see more.

github-actions · 2026-04-17T19:45:40Z

+      return;
+    }
+
+    odb::Point src = odb::Point(src_x, src_y);


warning: no header providing "odb::Point" is directly included [misc-include-cleaner]

src/dft/src/Dft.cpp:25:

- #include "utl/Logger.h" + #include "odb/geom.h" + #include "utl/Logger.h"

github-actions · 2026-04-17T19:45:41Z

+    odb::Point src = odb::Point(src_x, src_y);
+    odb::Point sink = odb::Point(sink_x, sink_y);
+
+    int64_t twl_internal = chain->estimateInternalTWL();


warning: no header providing "int64_t" is directly included [misc-include-cleaner]

src/dft/src/Dft.cpp:5:

- #include <memory> + #include <cstdint> + #include <memory>

github-actions · 2026-04-17T19:45:41Z

+    int64_t twl_internal = chain->estimateInternalTWL();
+    int64_t twl = twl_internal;
+    const auto& scan_cells = chain->getScanCells();
+    if (scan_cells.size() > 0) {


warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]

Suggested change

if (scan_cells.size() > 0) {

if (!scan_cells.empty()) {

Additional context

/usr/include/c++/13/bits/stl_vector.h:1087: method 'vector'::empty() defined here

empty() const _GLIBCXX_NOEXCEPT ^

github-actions · 2026-04-17T19:45:41Z

+          // Sort to reduce wire length
+          odb::Point f_src(src_x, src_y);
+          odb::Point f_sink(sink_x, sink_y);
+          if (rising.size() > 0) {


warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]

Suggested change

if (rising.size() > 0) {

if (!rising.empty()) {

Additional context

/usr/include/c++/13/bits/stl_vector.h:1087: method 'vector'::empty() defined here

empty() const _GLIBCXX_NOEXCEPT ^

github-actions · 2026-04-17T19:45:41Z

+              = OptimizeScanWirelength2Opt(f_src, f_sink, falling, logger_);
+
+          odb::Point r_src(src_x, src_y);
+          if (falling.size() > 0) {


warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]

Suggested change

if (falling.size() > 0) {

if (!falling.empty()) {

Additional context

/usr/include/c++/13/bits/stl_vector.h:1087: method 'vector'::empty() defined here

empty() const _GLIBCXX_NOEXCEPT ^

github-actions · 2026-04-17T19:45:42Z

+                                 utl::Logger* logger);
+
+int64_t OptimizeScanWirelength2Opt(
+    const odb::Point source,


warning: parameter 'source' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

const odb::Point source,

odb::Point source,

github-actions · 2026-04-17T19:45:42Z

+
+int64_t OptimizeScanWirelength2Opt(
+    const odb::Point source,
+    const odb::Point sink,


warning: parameter 'sink' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

const odb::Point sink,

odb::Point sink,

github-actions · 2026-04-17T19:45:42Z

+    const odb::Point sink,
+    std::vector<std::unique_ptr<ScanCell>>& cells,
+    utl::Logger* logger,
+    size_t max_iters = 30);


warning: no header providing "size_t" is directly included [misc-include-cleaner]

src/dft/src/architect/Opt.hh:5:

- #include <memory> + #include <cstddef> + #include <memory>

github-actions · 2026-04-17T19:45:42Z

+                = OptimizeScanWirelengthNN(falling, logger_);
+            auto rising_wire_length = OptimizeScanWirelengthNN(rising, logger_);
+            int64_t distance_between_falling_and_rising = 0;
+            if (rising.size() && falling.size()) {


warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]

Suggested change

if (rising.size() && falling.size()) {

if (!rising.empty() && falling.size()) {

Additional context

/usr/include/c++/13/bits/stl_vector.h:1087: method 'vector'::empty() defined here

empty() const _GLIBCXX_NOEXCEPT ^

github-actions · 2026-04-17T19:45:42Z

+                = OptimizeScanWirelengthNN(falling, logger_);
+            auto rising_wire_length = OptimizeScanWirelengthNN(rising, logger_);
+            int64_t distance_between_falling_and_rising = 0;
+            if (rising.size() && falling.size()) {


warning: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty]

Suggested change

if (rising.size() && falling.size()) {

if (rising.size() && !falling.empty()) {

Additional context

/usr/include/c++/13/bits/stl_vector.h:1087: method 'vector'::empty() defined here

empty() const _GLIBCXX_NOEXCEPT ^

donn · 2026-04-17T20:25:13Z

Anyone have advice on how I should make dbDft.reset "generated"

maliberty · 2026-04-18T05:17:16Z

@fgaray review if you have time.

maliberty

Needs a unit test

maliberty · 2026-04-18T05:36:39Z

 build_linux
 build_mac
 test/results
+/sandbox


why is this needed?

maliberty · 2026-04-18T05:42:23Z

+  return total_wire_length;
+}
+
+int64_t ScanChain::estimateInternalTWL()


TWL = total wire length?? A more descriptive name would be helpful

maliberty · 2026-04-18T05:52:03Z

+    return 0;
+  }
+
+  size_t N = cells.size() + 2;


use snake_case for variables, a more meaningful name, and make it const

maliberty · 2026-04-18T05:54:05Z

+
+  int64_t best_score = score;
+
+  // Already "optimal" (unplaced)


can't be unplaced as we already returned on unplaced above.

maliberty · 2026-04-18T05:54:54Z

+    const odb::Point sink,
+    std::vector<std::unique_ptr<ScanCell>>& cells,
+    utl::Logger* logger,
+    size_t max_iters)


donn · 2026-04-18T13:48:32Z

Will implement all suggestions when I get a moment 🫡

github-actions bot added the size/M label Apr 17, 2026

gemini-code-assist bot reviewed Apr 17, 2026

View reviewed changes

github-actions bot reviewed Apr 17, 2026

View reviewed changes

maliberty requested changes Apr 18, 2026

View reviewed changes

	if (rising.size() && falling.size()) {
	if (!rising.empty() && falling.size()) {

	if (rising.size() && falling.size()) {
	if (rising.size() && !falling.empty()) {

Conversation

donn commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of Change

Impact

Verification

Uh oh!

donn commented Apr 17, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

donn Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

QuantamHD Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

donn commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maliberty commented Apr 18, 2026

Uh oh!

maliberty left a comment

Choose a reason for hiding this comment

Uh oh!

maliberty Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

maliberty Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

maliberty Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

maliberty Apr 18, 2026

donn commented Apr 17, 2026 •

edited

Loading

donn commented Apr 17, 2026 •

edited

Loading