Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIRRTL] LowerAnnotations rejects Reset/UInt<1> Wiring Problem source/sink pairs #4651

Closed
fabianschuiki opened this issue Feb 13, 2023 · 1 comment · Fixed by #4656
Closed
Labels
FIRRTL Involving the `firrtl` dialect

Comments

@fabianschuiki
Copy link
Contributor

I stumbled across the following use of SourceAnnotation/SinkAnnotation in the wild which tries to wire an abstract Reset signal to a concrete UInt<1>:

circuit Foo : %[[
  {
    "class":"firrtl.passes.wiring.SourceAnnotation",
    "target":"~Foo|Bar>y",
    "pin":"xyz"
  },
  {
    "class":"firrtl.passes.wiring.SinkAnnotation",
    "target":"~Foo|Foo>x",
    "pin":"xyz"
  }
]]
  module Bar :
    wire y : Reset
    y is invalid

  module Foo :
    inst bar of Bar
    wire x : UInt<1>
    x is invalid

Running this through firtool --parse-only produces the following error:

input.fir:14:5: error: Wiring Problem source type '!firrtl.reset' does not match sink type '!firrtl.uint<1>'
    wire y : Reset
    ^
input.fir:19:5: note: The sink is here.
    wire x : UInt<1>
    ^

Related to #4496 which added support for lowering some of these legacy wiring annotations as WiringProblems.

The issue is that the LowerAnnotations pass already checks for the types of the source/sink to line up. But the abstract Reset type only gets eliminated in InferResets, which runs after this pass. So we either have to make the anno lowering more lenient (accepting compatible types), or move the type checking to a later point when we actually deal with materializing XMRs or the wires themselves.

@fabianschuiki fabianschuiki added the FIRRTL Involving the `firrtl` dialect label Feb 13, 2023
@dtzSiFive
Copy link
Contributor

Making wiring more lenient for compatible types (especially if no truncation/extension required) seems reasonable.

We already look ahead and insert a wire + cast for reset taps in similar situation:

// For resets, sometimes inject a cast between sink and target 'sink'.
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FIRRTL Involving the `firrtl` dialect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants