llvm / llvm/circt

[FIRRTL] InferResets FullReset (formerly FART) does not add resets to not reset fields of partially reset registers

Open
#7,675 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
2.2k
Forks
524
Avg merge
3d 2h
Merged PRs (30d)
46

Description

The original iteration of this bug was fixed in https://github.com/llvm/circt/pull/6912, but it appears that fix only works for registers in modules contain the wire or port marked with a `circt.FullResetAnnotation`, but does not work for children of the module with the annotated port or wire.

Consider the following FIRRTL:
```firrtl
FIRRTL version 3.3.0
; CHECK-LABEL: module test(
circuit top :%[[{
"class":"circt.FullResetAnnotation",
"target":"~top|top>reset",
"resetType":"async"
}]]
module test :
input clock : Clock
input reset : AsyncReset
input in : { foo : UInt<8>, bar : UInt<8>}
output out : { foo : UInt<8>, bar : UInt<8>}

wire reg1_w : { foo : UInt<8>, bar : UInt<8>}
invalidate reg1_w
; CHECK: reg1_foo <= 8'hC;
; CHECK: reg1_bar <= 8'h0;
connect reg1_w.foo, UInt<8>(0hc)
invalidate reg1_w.bar
; CHECK: reg1_foo = 8'hC;
; CHECK: reg1_bar = 8'h0;
regreset reg1 : { foo : UInt<8>, bar : UInt<8>}, clock, reset, reg1_w
wire reg2 : { foo : UInt<8>, bar : UInt<8>}
connect reg1, in
connect reg2, reg1
connect out, reg2

module top :
input clock : Clock
input reset : AsyncReset
input in : { foo : UInt<8>, bar : UInt<8>}
output out : { foo : UInt<8>, bar : UInt<8>}

inst child of test
connect child.clock, clock
connect child.reset, reset
connect child.in, in
connect out, child.out
```

Note that this is exactly the same as the first test in https://github.com/llvm/circt/blob/f75bbd7986d4d5ab254ce428492f0e2366c16055/test/firtool/async-reset.fir#L5, except that there is a new top module wrapping the module with registers, and the annotated port is in the top module.

Currently, `reg1.bar` does not properly get the expected async reset to `0`, but it should. If you change the annotation to target `~top|test>reset`, you will see the correct output.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with test/firtool/async-reset.fir and its first test, then compare it with the reproducer's added top wrapper and FullResetAnnotation target. Confirm the InferResets FullReset behavior when the annotation is on the parent module: reg1.bar should receive the expected asynchronous reset to 0.

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.