[FIRRTL] FART: not totally idempotent - but may be close enough
- Dominant language
- C++
- Stars
- 2.2k
- Forks
- 524
- Avg merge
- 3d 2h
- Merged PRs (30d)
- 46
Description
The FullAsynchronousResetTransform no longer removes its annotation, which can lead to problems where the pass is no longer idempotent. It seems like some work has gone in to minimizing the amount of work the pass does on subsequent runs, but there is at least one small issue, and possibly others.
given:
`/bin/circt-opt --pass-pipeline="builtin.module(firrtl.circuit(firrtl-infer-resets))"`
```mlir
firrtl.circuit "Foo" {
firrtl.module @Foo(in %p: !firrtl.uint<1>, in %r: !firrtl.asyncreset [{class = "sifive.enterprise.firrtl.FullAsyncResetAnnotation"}]) attributes {convention = #firrtl} {
%bar_clock = firrtl.instance bar @Bar(in clock: !firrtl.clock)
}
firrtl.module private @Bar(in %clock: !firrtl.clock) {
%r = firrtl.reg %clock : !firrtl.clock, !firrtl.uint<8>
}
}
```
the first run yields:
```mlir
firrtl.circuit "Foo" {
firrtl.module @Foo(in %p: !firrtl.uint<1>, in %r: !firrtl.asyncreset [{class = "sifive.enterprise.firrtl.FullAsyncResetAnnotation"}]) attributes {convention = #firrtl} {
%bar_r, %bar_clock = firrtl.instance bar @Bar(in r: !firrtl.asyncreset, in clock: !firrtl.clock)
firrtl.matchingconnect %bar_r, %r : !firrtl.asyncreset
}
firrtl.module private @Bar(in %r: !firrtl.asyncreset, in %clock: !firrtl.clock) {
%c0_ui8 = firrtl.constant 0 : !firrtl.const.uint<8>
%r_0 = firrtl.regreset %clock, %r, %c0_ui8 {name = "r"} : !firrtl.clock, !firrtl.asyncreset, !firrtl.const.uint<8>, !firrtl.uint<8>
}
}
```
the second run yields:
```mlir
firrtl.circuit "Foo" {
firrtl.module @Foo(in %p: !firrtl.uint<1>, in %r: !firrtl.asyncreset [{class = "sifive.enterprise.firrtl.FullAsyncResetAnnotation"}]) attributes {convention = #firrtl} {
%bar_r, %bar_clock = firrtl.instance bar @Bar(in r: !firrtl.asyncreset, in clock: !firrtl.clock)
firrtl.matchingconnect %bar_r, %r : !firrtl.asyncreset
firrtl.matchingconnect %bar_r, %r : !firrtl.asyncreset
}
firrtl.module private @Bar(in %r: !firrtl.asyncreset, in %clock: !firrtl.clock) {
%c0_ui8 = firrtl.constant 0 : !firrtl.const.uint<8>
%r_0 = firrtl.regreset %clock, %r, %c0_ui8 {name = "r"} : !firrtl.clock, !firrtl.asyncreset, !firrtl.const.uint<8>, !firrtl.uint<8>
}
}
```
Each run adds a new connect to the instance op. Although functionally equivalent, it would be best if the pass did nothing at all on the second run. We should audit other parts of this code to make sure that there aren't larger issues related to this. Not removing the annotation is required as SFCCompat needs to be aware of its existence.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start with the firrtl-infer-resets pipeline and FullAsynchronousResetTransform, then run the provided circt-opt reproducer twice and compare the IR. Trace why the second run adds another firrtl.matchingconnect while preserving the annotation. Done means the pass is unchanged on the second run and related idempotence issues in this code have been audited.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100