Skip to content

[FIRRTL][InferResets] Add InstanceChoice support#9955

Merged
uenoku merged 3 commits intollvm:mainfrom
uenoku:dev/hidetou/infer-reset-choice
Mar 21, 2026
Merged

[FIRRTL][InferResets] Add InstanceChoice support#9955
uenoku merged 3 commits intollvm:mainfrom
uenoku:dev/hidetou/infer-reset-choice

Conversation

@uenoku
Copy link
Copy Markdown
Member

@uenoku uenoku commented Mar 16, 2026

Extend the InferResets pass to handle InstanceChoiceOp alongside
InstanceOp. This enables reset type inference to work correctly
across instance_choice operations by tracing resets through all
possible target modules. The implementation uses InstanceOpInterface
and extracts common logic into a template helper function.

AI-assited-by: Sonnet 4.5

@uenoku uenoku changed the title Add InstanceChoice support to InferResets pass [FIRRTL] Add InstanceChoice support to InferResets pass Mar 16, 2026
@uenoku uenoku marked this pull request as draft March 16, 2026 22:35
Comment thread lib/Dialect/FIRRTL/Transforms/InferResets.cpp Outdated
@uenoku uenoku force-pushed the dev/hidetou/infer-reset-choice branch 3 times, most recently from f1d5f68 to aa5460a Compare March 17, 2026 08:09
@uenoku uenoku marked this pull request as ready for review March 17, 2026 08:12
@uenoku uenoku changed the title [FIRRTL] Add InstanceChoice support to InferResets pass [FIRRTL][InferResets] Add InstanceChoice support Mar 17, 2026
Copy link
Copy Markdown
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

Generally LTGM and mostly nits. Two questions:

  1. Any way to remove the template? That always feels a bit hacky or it indicates that the IR could be improved or generalized.
  2. Tests of errors should be added.

Comment thread lib/Dialect/FIRRTL/Transforms/InferResets.cpp Outdated
Comment thread lib/Dialect/FIRRTL/Transforms/InferResets.cpp Outdated
Comment thread lib/Dialect/FIRRTL/Transforms/InferResets.cpp
Comment on lines +920 to +921
if (!node || !node->getModule())
return;
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.

I forget. What condition is this trying to catch?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's redundant and deleted. Thank you for pointing out.

Comment thread lib/Dialect/FIRRTL/Transforms/InferResets.cpp Outdated
Comment thread lib/Dialect/FIRRTL/Transforms/InferResets.cpp Outdated
Comment thread lib/Dialect/FIRRTL/Transforms/InferResets.cpp Outdated
Comment thread lib/Dialect/FIRRTL/Transforms/InferResets.cpp Outdated
Comment thread test/Dialect/FIRRTL/infer-resets-errors.mlir
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.

Can the PR include an error test?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, added!

uenoku added 2 commits March 20, 2026 22:38
This commit adds comprehensive support for InstanceChoiceOp in the
InferResets pass, handling all referenced modules (default target and
all alternatives).
@uenoku uenoku force-pushed the dev/hidetou/infer-reset-choice branch from aa5460a to 39130ad Compare March 21, 2026 08:43
@uenoku uenoku merged commit c7e6d61 into llvm:main Mar 21, 2026
7 checks passed
@uenoku uenoku deleted the dev/hidetou/infer-reset-choice branch March 21, 2026 09:27
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.

2 participants