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

Rewrite BlackBox IO contract, replace _clock & _reset #156

Closed
wants to merge 3 commits into from

Conversation

sdtwigg
Copy link
Contributor

@sdtwigg sdtwigg commented Apr 27, 2016

The old blackbox behavior still emitted extmodules that have a
clk, reset pin and prepended all io's with io_ (ultimately). Most
verilog modules do not follow this distinction (or use a slightly
different name for clock and so on).

Thus, instead BlackBox has been rewritten to not assume a clk or
reset pin. Instead, the io Bundle specified is flattened directly
into the Module.ports declaration. The tests have been rewritten
to compensate for this. Also, added a test that uses the clock pin.

As a secondary change, the _clock and _reset module parameters were
bad for two reasons. One, they used null as a default, which is a
scala best practices violation. Two, they were just not good names.

Instead the primary constructor has been rewritten to take an
Option[Clock] called override_clock and an Option[Bool] called
override_reset, which default to None. (Note how the getOrElse call
down below is much more natural now.)

However, users may not want to specify the Some(their_clock) so I
also added secondary constructors that take parameters named clock
and reset and wrap them into Some calls into the primary constructor.
This is a better UX because now you can just stipulate clock=blah in
instantiation of that module in symmetry with using the clock in the
definition of the module by invoking clock.

PS: We could also back out of allowing any overrides via the Module
constructor and just require the instantiating Module to do
submodule.clock := newclock, etc.

The old blackbox behavior still emitted extmodules that have a
clk, reset pin and prepended all io's with io_ (ultimately). Most
verilog modules do not follow this distinction (or use a slightly
different name for clock and so on).

Thus, instead BlackBox has been rewritten to not assume a clk or
reset pin. Instead, the io Bundle specified is flattened directly
into the Module.ports declaration. The tests have been rewritten
to compensate for this. Also, added a test that uses the clock pin.

As a secondary change, the _clock and _reset module parameters were
bad for two reasons. One, they used null as a default, which is a
scala best practices violation. Two, they were just not good names.

Instead the primary constructor has been rewritten to take an
Option[Clock] called override_clock and an Option[Bool] called
override_reset, which default to None. (Note how the getOrElse call
down below is much more natural now.)

However, users may not want to specify the Some(their_clock) so I
also added secondary constructors that take parameters named clock
and reset and wrap them into Some calls into the primary constructor.
This is a better UX because now you can just stipulate clock=blah in
instantiation of that module in symmetry with using the clock in the
definition of the module by invoking clock.

PS: We could also back out of allowing any overrides via the Module
constructor and just require the instantiating Module to do
submodule.clock := newclock, etc.
@aswaterman
Copy link
Member

The _reset = null thing is a Scala anti-pattern, but it is part of the API, and we need to support this feature in both chisel2 and chisel3 simultaneously.

@sdtwigg
Copy link
Contributor Author

sdtwigg commented Apr 27, 2016

You can change the secondary constructors argument names to _reset and _clock and still remove the null.

However, you could keep chisel2 and chisel3 simultaneous operation (for the very small set of circuits that needs to compile on both) by porting the change to Chisel2 as well.

Edit: Alternatively, can just mark all the secondary constructors as deprecated and tell the users to manually assign .clock and .reset from the parent in chisel3.

@aswaterman
Copy link
Member

I can't change the chisel2 API for _reset etc. without an act of congress, and it's a showstopper to have no means to compile these circuits in both chisel2 and chisel3. So I can't merge the patch as-is.

@aswaterman
Copy link
Member

I'm otherwise supportive, so if you can resubmit so it doesn't break code that relies on the old regime, we can continue.

Stephen Twigg added 2 commits April 27, 2016 17:06
@sdtwigg
Copy link
Contributor Author

sdtwigg commented Apr 28, 2016

PTAL

I don't think current tests exercise the_clock or _reset functionality but I did test to ensure this compiled:

class MyModule(rs: Bool) extends Module(_reset=rs) {
  val io = new Bundle {
    val out = Bool(OUTPUT)
  }
}

Note, since reset can be overridden manually. e.g in parent do:
child.reset := newResetForChild
it is tempting to mark this type of reset behavior as deprecated. (Since, the current behavior expects the child to say when its reset can be overridden when that is likely a decision the parent wished to make)

@aswaterman
Copy link
Member

Yeah, that works for me. Thanks.

I agree child.reset := newReset is better, as the child need not indicate its willingness to have its reset overridden. Likewise clock.

If no one else has objections, I'm OK to merge--though I'm sure this won't be the final word on BlackBox.

@aswaterman
Copy link
Member

Seems that the ayes have it.

@aswaterman aswaterman closed this in f3ea8f1 May 4, 2016
@aswaterman aswaterman mentioned this pull request May 4, 2016
ucbjrl pushed a commit that referenced this pull request Mar 9, 2018
Fix several classes to use ActualDirection/DataMirror.directionOf API
Fixed up Flip and UInt literal api references in several tests
mwachs5 pushed a commit that referenced this pull request Dec 29, 2022
Bumps [chisel3](https://github.com/freechipsproject/chisel3) from `9b54b64` to `61fbf23`.
- [Release notes](https://github.com/freechipsproject/chisel3/releases)
- [Commits](9b54b64...61fbf23)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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

Successfully merging this pull request may close these issues.

2 participants