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

Chisel chokes on using elements of an unbound Aggregate as elements of a Record #4215

Open
jackkoenig opened this issue Jun 24, 2024 · 2 comments

Comments

@jackkoenig
Copy link
Contributor

Type of issue: Bug Report

Please provide the steps to reproduce the problem:

Consider the following Chisel:

//> using scala "2.13.12"
//> using dep "org.chipsalliance::chisel:6.4.0"
//> using plugin "org.chipsalliance:::chisel-plugin:6.4.0"
//> using options "-unchecked", "-deprecation", "-language:reflectiveCalls", "-feature", "-Xcheckinit", "-Xfatal-warnings", "-Ywarn-dead-code", "-Ywarn-unused", "-Ymacro-annotations"

import chisel3._
// _root_ disambiguates from package chisel3.util.circt if user imports chisel3.util._
import _root_.circt.stage.ChiselStage
import scala.collection.immutable.VectorMap

class MyRecord extends Record {
  val a = UInt(8.W)
  val b = Vec(2, UInt(8.W))
  val elements = VectorMap("a" -> a) ++ b.zipWithIndex.map { case (e, i) => i.toString -> e }
}

class Foo extends Module {
  val in = IO(Input(new MyRecord))
  val out = IO(Output(new MyRecord))

  out := in
}

object Main extends App {
  println(
    ChiselStage.emitCHIRRTL(new Foo)
  )
}

What is the current behavior?

This emits:

circuit Foo :
  module Foo :
    input clock : Clock
    input reset : UInt<1>
    input in : { [ILit(1)] : UInt<8>, [ILit(0)] : UInt<8>, a : UInt<8>}
    output out : { [ILit(1)] : UInt<8>, [ILit(0)] : UInt<8>, a : UInt<8>}

    connect out, in

The input in : { [ILit(1)] : UInt<8>, [ILit(0)] : UInt<8>, a : UInt<8>} is nonsensical.

What is the expected behavior?

It should either error or emit valid FIRRTL

Note there are other ways that this same bug can manifest. Trying to dontTouch in or out throws an exception pointing to Chisel internals.

Please tell us about your environment:

Other Information

What is the use case for changing the behavior?

@mwachs5
Copy link
Contributor

mwachs5 commented Jun 24, 2024

is this only for Vecs or does it manifest for other Bundles and Record you might try to do this to

@jackkoenig
Copy link
Contributor Author

jackkoenig commented Jun 24, 2024

is this only for Vecs or does it manifest for other Bundles and Record you might try to do this to

I assumed it would apply to all Aggregates, but it seems to be only for Vecs. The issue is that the ref for the children of Vecs is unconditionally pointing to the Vec within which they are defined[1] (before binding), whereas for Records it is set during binding[2].

Regardless, I think we should ban this because it's a pretty sharp edge even when it "works". The user may reasonably see b as a thing that they can use, connect to it to just connect to those fields. They will expect it to act like a view, but it will not. Things will work if you just iterate on the elements of b, but will fail if you try to do anything with b itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants