Skip to content

[Support] Add parallel instance graph visitors#10353

Draft
seldridge wants to merge 4 commits into
mainfrom
dev/seldridge/instance-graph-parallel-walkers
Draft

[Support] Add parallel instance graph visitors#10353
seldridge wants to merge 4 commits into
mainfrom
dev/seldridge/instance-graph-parallel-walkers

Conversation

@seldridge
Copy link
Copy Markdown
Member

@seldridge seldridge commented Apr 29, 2026

Add parallel versions of post/inverse post order walkers that exist for
the InstanceGraph. This is added to hopefully extract more parallelism
from existing passes that need to visit "parents before children" or
"children before parents", but can visit all parents or all children in
parallel, respectively.

Assisted-by: pi.dev:claude-sonnet-4-6
Assisted-by: pi.dev:claude-opus-4-7

Warning: this is presently marginally-reviewed LLM output. Once I have sufficiently reviewed this I will mark this as non-draft.

Add parallel versions of post/inverse post order walkers that exist for
the InstanceGraph.  This is added to hopefully extract more parallelism
from existing passes that need to visit "parents before children" or
"children before parents", but can visit all parents or all children in
parallel, respectively.

Assisted-by: pi.dev:claude-sonnet-4-6
Assisted-by: pi.dev:claude-opus-4-7
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
@seldridge seldridge force-pushed the dev/seldridge/instance-graph-parallel-walkers branch from c2643ca to 47f18a3 Compare April 29, 2026 04:23
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
@fabianschuiki
Copy link
Copy Markdown
Contributor

Results of circt-tests run for 5038924 compared to results for 926f5ea: no change to test results.

/// been visited (and their callbacks completed) before the node itself is
/// visited, but sibling nodes may be visited concurrently.
///
/// **Thread safety:** The callback `fn` must be safe to invoke from
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.

Please avoid duplication or emphasize the difference if any

/// Defined out-of-line in InstanceGraph.cpp to keep the header free of
/// <atomic>, ThreadPool, and Threading includes.
LogicalResult
walkParallelImpl(llvm::function_ref<LogicalResult(InstanceGraphNode &)> fn,
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.

Maybe decorate constant to ensure mutation requirement with type if that works?

Suggested change
walkParallelImpl(llvm::function_ref<LogicalResult(InstanceGraphNode &)> fn,
walkParallelImpl(llvm::function_ref<LogicalResult(const InstanceGraphNode &)> fn,

@fabianschuiki
Copy link
Copy Markdown
Contributor

This looks really cool! Nice use of the task group API and having a task spawn additional tasks into the group 💯

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants