llvm / llvm/circt

[FIRRTL] FART: when annotated reset is under a when, always leads to uninitialized wires

Open
#7,274 0 comments 0 reactions 0 assignees View on GitHub
FIRRTL
Dominant language
C++
Stars
2.2k
Forks
524
Avg merge
3d 2h
Merged PRs (30d)
46

Description

There is some logic when wiring the annotated reset to target registers, that we try to find a location to create a reset wire which dominates all use-sites. The problem is that there is no situation where this logic kicks in that won't later yield an unitialized signal error later on the pipeline.
```firrtl
FIRRTL version 4.0.0
circuit Foo: %[[
{"class":"sifive.enterprise.firrtl.FullAsyncResetAnnotation", "target":"~Foo|Foo>n"}
]]
public module Foo:
input p : UInt<1>
input r : AsyncReset
input c : Clock
when p:
node n = r
reg reg : UInt<8>, c
```
yields
```mlir
// -----// IR Dump After InferResets (firrtl-infer-resets) //----- //
firrtl.circuit "Foo" {
firrtl.module @Foo(in %p: !firrtl.uint<1>, in %r: !firrtl.asyncreset, in %c: !firrtl.clock) attributes {convention = #firrtl} {
%n = firrtl.wire : !firrtl.asyncreset
firrtl.when %p : !firrtl.uint<1> {
%n_0 = firrtl.node %r {annotations = [{class = "sifive.enterprise.firrtl.FullAsyncResetAnnotation"}], name = "n"} : !firrtl.asyncreset
firrtl.matchingconnect %n, %n_0 : !firrtl.asyncreset
}
%c0_ui8 = firrtl.constant 0 : !firrtl.const.uint<8>
%reg = firrtl.regreset %c, %n, %c0_ui8 : !firrtl.clock, !firrtl.asyncreset, !firrtl.const.uint<8>, !firrtl.uint<8>
}
}
```
A wire is created in the outmost scope, but only driven inside the WhenOp.
During ExpandWhens, we hit the error:
```
./fart7.fir:10:7: error: sink "n" not fully initialized in "Foo"
node n = r
```
As it stands, I don't think there is any way that the responsible logic can kick in and yield a circuit that will make it past ExpandWhens, making it rather useles.

A potential solution might be to invalidate the wire, which we know expand whens will lower to `mux(p, n, DontCare) => n`, but this is not guaranteed to always be the case, as in the future we hope to treat invalid values as they described in the spec. We could even optimize the DontCare to a time-varying signal.

A better solution might be to add a static connect operation which is not affected by containing when operations (h/t @dtzSiFive ).

This is heavily related to https://github.com/llvm/circt/issues/7264.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by tracing the InferResets and ExpandWhens entry points using the FIRRTL example in this issue, then compare the related behavior in issue #7264. Determine how an annotated reset under a when can avoid producing an uninitialized wire, and verify that the circuit passes ExpandWhens without the reported error.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.