[FIRRTL] FullAsyncResetTransform: does not properly find dominating block to insert reset signal
- Dominant language
- C++
- Stars
- 2.2k
- Forks
- 524
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 46
Description
Given:
```fir
FIRRTL version 3.0.0
circuit Foo: %[[
{"class":"sifive.enterprise.firrtl.FullAsyncResetAnnotation", "target":"~Foo|Foo>reset"}
]]
module Foo:
input p : UInt<1>
input r : AsyncReset
when p:
wire reset : AsyncReset
inst bar of Bar
inst bar of Bar
module Bar:
```
which if compiled with `firtool test.fir` gives the following error:
```
./fart2.fir:11:5: error: operand #1 does not dominate this use
inst bar of Bar
^
./fart2.fir:11:5: note: see current operation: "firrtl.matchingconnect"(%0, %1) : (!firrtl.asyncreset, !firrtl.asyncreset) -> ()
./fart2.fir:9:7: note: operand defined here (op in a child region)
wire reset : AsyncReset
```
The issue is that it attempts to connect the wire `reset` to an input port the second instance of `bar`. There is logic for handling things of this sort, but it seems to only care about the _first_ operation we need to wire the signal to, instead of looking at all operations.
https://github.com/llvm/circt/blob/281dc6cfa3638a4f77aba9a9f07bf2193bdc3959/lib/Dialect/FIRRTL/Transforms/InferResets.cpp#L1730-L1760
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in lib/Dialect/FIRRTL/Transforms/InferResets.cpp around lines 1730-1760, where FullAsyncResetTransform chooses the dominating block for reset wiring. Reproduce the failure with the FIRRTL example from the issue using firtool, then verify that the reset signal is inserted where it dominates every operation that uses it and that the reported dominance error is gone.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100