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

Bulk connect on BlackBox io doesn't work for Bundles defined compatibility mode #870

Open
jackkoenig opened this issue Aug 9, 2018 · 1 comment
Labels
Milestone

Comments

@jackkoenig
Copy link
Contributor

Note that the title includes BlackBoxes defined in import chisel3._ code that use Bundles defined in import Chisel._ code, so this bug percolates into attempted migration to chisel3._ as well. You can connect to the fields of BlackBox ios just fine, but bulk connecting to the full aggregate doesn't work.

I'm almost certain the bug comes from the special case handling of BlackBox io. Since those Bundles are just kind of shims for "top-level" ports, the Bundle doesn't actually exist in the FIRRTL. And due to #595, bulk connect semantics for compatibility mode defined bundles take effect even when used in chisel3._ code.

Proposed solutions:

  1. Hacky support for exactly this case--detect when the Bundle involved in a bulk connect is a BlackBox io and do something different
    • Adding a FIRRTL mechanism for bulk connecting to an instance could make the implementation of the bulk connect clean and provide a potentially useful Chisel API, but the detection is still one off
  2. Generalized support for "shim" Bundles that actually inject their elements into their outer context
    • This could provide a reasonable solution for setName equivalent in Chisel3 #612 although I suspect actual "set name" support is still desirable
    • Unclear how this would work for a Vec of shim Bundles, maybe that would be illegal but shim Bundles would be legal in other Bundles and as IO?
  3. Change the mechanism for BlackBoxes to use a FIRRTL "set name" API so that the Bundles are no longer shims

Type of issue: bug report

Impact: no functional change

Development Phase: request

Other information

If the current behavior is a bug, please provide the steps to reproduce the problem:

You can run the following code:

import Chisel._

class MyBlackBox extends BlackBox {
  val io = IO(new Bundle {
    val in = Input(UInt(8.W))
    val out = Output(UInt(8.W))
  })
}

class TopModule extends Module {
  val io = IO(new Bundle {
    val in = Input(UInt(8.W))
    val out = Output(UInt(8.W))
  })

  val inst = Module(new MyBlackBox)
  io <> inst.io
}

object Top extends App {
  println(chisel3.Driver.emit(() => new TopModule))
}

What is the current behavior?

And it prints:

;buildInfoPackage: chisel3, version: 3.2-SNAPSHOT, scalaVersion: 2.11.12, sbtVersion: 1.1.1
circuit TopModule : 
  extmodule MyBlackBox : 
    output out : UInt<8>
    input in : UInt<8>
    
    defname = MyBlackBox
    
    
  module TopModule : 
    input clock : Clock
    input reset : UInt<1>
    output io : {flip in : UInt<8>, out : UInt<8>}
    
    clock is invalid
    reset is invalid
    io is invalid
    inst inst of MyBlackBox @[Test.scala 67:20]
    inst.out is invalid
    inst.in is invalid
    io <- ?? @[Test.scala 68:6]

Clearly the io <- ?? is wrong. Prior to the literal refactor, this errored with a None.get in the emitter.

What is the expected behavior?

Obviously bulk connecting with the io of a BlackBox should work.

What is the use case for changing the behavior?

🐛🔨

@jackkoenig jackkoenig added the bug label Aug 9, 2018
@jackkoenig jackkoenig added this to the 3.2.0 milestone Aug 9, 2018
@jackkoenig
Copy link
Contributor Author

h/t to @p12nGH for discovery

@chick chick modified the milestones: 3.2.0, 3.3.0 Dec 17, 2018
@azidar azidar modified the milestones: 3.3.0, 3.4.x Oct 27, 2020
jackkoenig pushed a commit that referenced this issue Feb 28, 2023
Major features:

- Added Interval type, as well as PrimOps asInterval, clip, wrap, and sqz.
- Changed PrimOp names: bpset -> setp, bpshl -> incp, bpshr -> decp
- Refactored width/bound inferencer into a separate constraint solver
- Added transforms to infer, trim, and remove interval bounds
- Tests for said features

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

No branches or pull requests

3 participants