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

RFC: Add Rocket Chip-style clonemodule as CloneAndGetIO to experimental #943

Merged
merged 4 commits into from
Jan 22, 2019

Conversation

albert-magyar
Copy link
Contributor

@albert-magyar albert-magyar commented Nov 16, 2018

Related issue: #618
Type of change: feature request
Impact: API addition (no impact on existing code)
Development Phase: proposal && implementation

EXAMPLE USAGE

val q1 = Module(new Queue(...))
val q2_io = CloneAndGetIO(q1)("io").asInstanceOf[q1.io.type]
q2_io.enq <> q1.io.deq

Release Notes:
This is a port of Rocket Chip's CloneModule functionality (which returns a Record) as chisel3.experimental.CloneAndGetIO. This simplifies the implementation drastically, as it can live inside chisel3. The idea here is that there may be some desire to have this live alongside a full CloneModule implementation that returns a Module, as the API for using the Record-returning version is much simpler for basic use cases.

Pros: it makes the simple use cases simple. Also, I am almost positive that anything returning something more module-like would have also have large, non-overlapping caveats associated with its use, which incentivizes having both variants.
Cons: it might be confusing to have both CloneAndGetIO and another version of CloneModule that returns a Module. Also, more API features means more to support ad infinitum.

@albert-magyar albert-magyar requested a review from a team as a code owner November 16, 2018 21:23
@albert-magyar albert-magyar added Feature New feature, will be included in release notes DO NOT MERGE Request For Comment labels Nov 16, 2018
@edwardcwang
Copy link
Contributor

Are there some tests to show how this is supposed to work for users? How does this interact with the full CloneModule proposal you brought up a few weeks ago?

@azidar
Copy link
Contributor

azidar commented Nov 21, 2018

@albert-magyar I agree with @edwardcwang , can we see some tests demonstrating the new API?

@albert-magyar albert-magyar force-pushed the rc-clonemodule branch 2 times, most recently from 50051a1 to 358109a Compare November 30, 2018 02:57
@albert-magyar
Copy link
Contributor Author

@azidar @edwardcwang I added some Scaladoc / tests.

@albert-magyar
Copy link
Contributor Author

The idea is that can co-exist alongside a CloneModule that returns a Module. This is intrinsically safer and easier to use in simple cases, and will be even faster than one that returns a Module since there is no need to shallow-copy / initialize / do anything with random public vals.

@aswaterman
Copy link
Member

@terpstra FYI

src/main/scala/chisel3/package.scala Outdated Show resolved Hide resolved
src/test/scala/chiselTests/CloneModuleSpec.scala Outdated Show resolved Hide resolved
src/test/scala/chiselTests/CloneModuleSpec.scala Outdated Show resolved Hide resolved
@edwardcwang
Copy link
Contributor

Thanks Albert for the tests/examples. Just added some additional questions.

@albert-magyar albert-magyar force-pushed the rc-clonemodule branch 3 times, most recently from 78300f3 to 8210c3f Compare December 7, 2018 21:28
@albert-magyar
Copy link
Contributor Author

@stevenmburns (sorry I should have tagged you earlier), it would be interesting to see if this would work for your use case.

@stevenmburns
Copy link

This looks good to me. I can use it to do what I want this way:

package pg

import chisel3._
import chisel3.experimental.{CloneAndGetIO}

class And extends Module {
  val io = IO(new Bundle {
    val a = Input( Bool())
    val b = Input( Bool())
    val z = Output( Bool())
  })

  io.z := io.a & io.b
}

class AndVec extends Module {
  val io = IO(new Bundle {
    val a = Input( Vec(8, Bool()))
    val b = Input( Vec(8, Bool()))
    val z = Output( Vec(8, Bool()))
  })

  val template = Module( new And())

  for { ((a,b,z),idx) <- ((io.a,io.b,io.z).zipped).toList.zipWithIndex} {
     val instance_io = if ( idx == 0) {
       template.suggestName( s"And_${idx}")
       template.io 
     } else {
       val instance = CloneAndGetIO( template)
       instance.suggestName( s"And_${idx}")
       instance("io").asInstanceOf[template.io.type]
     }
     instance_io.a := a
     instance_io.b := b
     z := instance_io.z
  }

}

object Driver extends App {
  chisel3.Driver.execute(args, () => new AndVec())
}

It is a little bit of a pain to have to do the first instance a little differently than the rest. I tried to do all eight using CloneModuleAndGetIO, but then template was dangling (not connected) and produced a firrtl error.

Also, the name is probably not correct. Maybe CloneModuleAndGetRecord or CloneModuleAsRecord.

@albert-magyar
Copy link
Contributor Author

@stevenmburns, I actually am out today for the development meeting, but I talked to @jackkoenig, and he’ll review after I finalize the PR today. It will be in this weekend.

@jackkoenig jackkoenig added this to the 3.2.0 milestone Jan 18, 2019
Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@albert-magyar albert-magyar merged commit 26660ff into chipsalliance:master Jan 22, 2019
@albert-magyar
Copy link
Contributor Author

Resolves #618

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE Feature New feature, will be included in release notes Request For Comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants