Chunk vertices in BFS parallel iteration for dynamic load balancing

[calewis]

Dispatch fixed-size chunks (8 vertices per task) in BfsIterator::visitParallel instead of one large contiguous slice per thread. Small chunks let threads that finish early pick up more work, and passing the level vertices to tasks by reference removes a per-task copy of the whole vector.

These changes can be thrown out in future upstream merges if there are conflicts. This code is a minor improvement, meant to help out a bit until future upstream bfs improvements land.

OpenSTA measurements

report_checks on a 32-core machine (min of 5 runs):

• gcd_sky130hd, 32 threads: 1845 us -> 1255 us (-32%)
• gcd_sky130hd, 8 threads: 1254 us -> 1117 us (-11%)
• aes_nangate45 (~17k inst): neutral at 8 and 32 threads
• Single-threaded code path is unchanged.

ORFS validation (OpenROAD master, base vs. this branch, NUM_CORES=8)

Every stage .odb is byte-identical (sha1) between the two binaries; flow wall time is neutral.

Design	base	branch
nangate45/gcd	19 s	19 s
nangate45/aes	225 s	223 s
nangate45/jpeg	687 s	678 s
sky130hd/aes	979 s	972 s

STA micro-benchmark on the final ORFS databases

Design	Threads	base	branch	delta
nangate45/jpeg (~70k inst)	8	238 ms	211 ms	-11%
nangate45/jpeg	32	256 ms	253 ms	-1%
sky130hd/aes	8	63 ms	61 ms	-3%
sky130hd/aes	32	75 ms	74 ms	0%

[CLAassistant]

All committers have signed the CLA.

[gemini-code-assist]

Code Review

This pull request updates the parallel BFS implementation in search/Bfs.cc to chunk vertices into small, fixed-size tasks for dynamic load balancing across worker threads. While this improves performance, indexing into the visitors array using the thread_id from the dispatch queue introduces a potential out-of-bounds access if the queue's thread count diverges from thread_count_. It is recommended to size visitors using dispatch_queue_->getThreadCount() and add an assertion to verify that thread_id is within bounds.

[calewis]

@dsengupta0628 I don't think the test failures are from my changes but from updating the regression tests to use bazel value. BUT I'm not 100% sure about that, the tests do all pass locally (bazel build) and I can't see how these changes would cause minor QOR differences.

How should we proceed?

[dsengupta0628]

@dsengupta0628 I don't think the test failures are from my changes but from updating the regression tests to use bazel value. BUT I'm not 100% sure about that, the tests do all pass locally (bazel build) and I can't see how these changes would cause minor QOR differences.

How should we proceed?

The best way would be to file an OpenROAD PR that bumps sta pointer to this change's commit and let the CI run there. There Bazel build based tests run, so if this is QoR neutral, regressions should pass. It is guaranteed to pick your change only in sta as there has been no commit after the latest sta master 8/14 upstream code merge- on top of which your change is built.

[calewis]

The best way would be to file an OpenROAD PR that bumps sta pointer to this change's commit and let the CI run there. There Bazel build based tests run, so if this is QoR neutral, regressions should pass. It is guaranteed to pick your change only in sta as there has been no commit after the latest sta master 8/14 upstream code merge- on top of which your change is built.

I can do that but needing to open a dummy PR in another project seems like a poor experience for anyone trying to submit PRs to this repo.

[dsengupta0628]

The best way would be to file an OpenROAD PR that bumps sta pointer to this change's commit and let the CI run there. There Bazel build based tests run, so if this is QoR neutral, regressions should pass. It is guaranteed to pick your change only in sta as there has been no commit after the latest sta master 8/14 upstream code merge- on top of which your change is built.

I can do that but needing to open a dummy PR in another project seems like a poor experience for anyone trying to submit PRs to this repo.

Why would it be a dummy PR? This change would need to propagate to OpenROAD anyways, so the src/sta pointer needs to have this change too for OpenROAD to be able to see that. But yes, not having Bazel build based regression testing here is a limitation for immediate review - please note this is a fairly new repo and upstream does not have any Bazel build based testing anyways. Plus the OpenROAD already has the capability- so we never face any issues trying to resolve Bazel/cmake discrepancies typically.

[calewis]

Dummy was the wrong term, sorry. My frustration is that this is a clunky way to run the tests, shouldn't this repo be able to test itself effectively?

Here is the OR PR The-OpenROAD-Project/OpenROAD#11243

[dsengupta0628]

Dummy was the wrong term, sorry. My frustration is that this is a clunky way to run the tests, shouldn't this repo be able to test itself effectively?

Here is the OR PR The-OpenROAD-Project/OpenROAD#11243

I hear ya'! Let me see if we can somehow get this resolved so that the tests can be run seamlessly.

[calewis]

@dsengupta0628 looks like the upstream PR passed.

[calewis]

Do I need to do anything else?

[dsengupta0628]

We merge upstream OpenSTA regularly, so please keep the diff to upstream code as small as possible and isolate the new logic in a marked block.

1. Please drop the visit() comment rewrite, the #include , and the visitors sizing change - I dont think that is needed for the feature and each is a guaranteed conflict if upstream touches those lines.
2. Move the chunked dispatch into a new BfsIterator::visitLevelChunked() (declaration in Bfs.hh, definition at the end of the BfsIterator section in Bfs.cc), both wrapped in // ---- OpenROAD fork: BFS chunked dispatch (begin/end) ---- markers. Replace only the body of the existing else branch in visitParallel with one marked call.
3. Keep upstream's vertex_count < thread_count fallback unless measurements say otherwise; if you need the 3-chunk threshold, put it inside the helper.

[dsengupta0628]

Recommended shape:

Principle: upstream lines stay byte-identical; fork logic lives in a marked, self-contained block, called from one marked line.

include/sta/Bfs.hh — inside class BfsIterator, protected section:

  // ---- OpenROAD fork: BFS chunked dispatch (begin) ----
  // Dispatch level_vertices to the worker pool in fixed-size chunks so
  // idle threads pick up remaining work. Stop-gap until upstream BFS
  // rework lands; on merge conflict take upstream and drop this.
  void visitLevelChunked(VertexSeq &level_vertices,
                         Level level,
                         std::vector<VertexVisitor *> &visitors);
  // ---- OpenROAD fork: BFS chunked dispatch (end) ----

search/Bfs.cc — in visitParallel, replace only the body of the existing else branch:

   else {
            // ---- OpenROAD fork: BFS chunked dispatch (begin) ----
            visitLevelChunked(level_vertices, level, visitors);
            // ---- OpenROAD fork: BFS chunked dispatch (end) ----
          }

and add the implementation at the end of the BfsIterator section (before //// + BfsFwdIterator), fully wrapped:

// ---- OpenROAD fork: BFS chunked dispatch (begin) ----
void
BfsIterator::visitLevelChunked(VertexSeq &level_vertices,
                               Level level,
                               std::vector<VertexVisitor *> &visitors)
{
  // Tasks read level_vertices in place and unlocked. This relies on
  // visitors never enqueuing at the current level (they only enqueue
  // fanout/fanin, which levelize places at a higher/lower level).
  // Chunk size vs. runtime is U-shaped; 8 is the smallest size at the
  // bottom of the curve.
  constexpr size_t chunk_size = 8;
  size_t vertex_count = level_vertices.size();
  BfsIndex bfs_index = bfs_index_;
  for (size_t from = 0; from < vertex_count; from += chunk_size) {
    size_t to = (from + chunk_size < vertex_count) ? from + chunk_size : vertex_count;
    dispatch_queue_->dispatch([this, &level_vertices, from, to, level,
                               bfs_index, &visitors](int thread_id) {
      VertexVisitor *thread_visitor = visitors[thread_id];
      for (size_t i = from; i < to; i++) {
        Vertex *vertex = level_vertices[i];
        if (vertex) {
          checkLevel(vertex, level);
          vertex->setBfsInQueue(bfs_index, false);
          thread_visitor->visit(vertex);
        }
      }
    });
  }
  dispatch_queue_->finishTasks();
}
// ---- OpenROAD fork: BFS chunked dispatch (end) ----

[dsengupta0628]

Do I need to do anything else?

Yes please :) I added the comment below.

[calewis]

I updated the OR main repo test pr as well to make sure those pass. The-OpenROAD-Project/OpenROAD#11243