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] [Proposal] Programmatic Port Creation #833

Merged
merged 11 commits into from
Jun 21, 2018
56 changes: 40 additions & 16 deletions chiselFrontend/src/main/scala/chisel3/core/Module.scala
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,40 @@ object Module {
def currentModule: Option[BaseModule] = Builder.currentModule
}

object IO {
/** Constructs a port for the current Module
*
* This must wrap the datatype used to set the io field of any Module.
* i.e. All concrete modules must have defined io in this form:
* [lazy] val io[: io type] = IO(...[: io type])
*
* Items in [] are optional.
*
* The granted iodef must be a chisel type and not be bound to hardware.
*
* Also registers a Data as a port, also performing bindings. Cannot be called once ports are
* requested (so that all calls to ports will return the same information).
* Internal API.
*/
def apply[T<:Data](iodef: T): T = {
val module = Module.currentModule.get // Impossible to fail
require(!module.isClosed, "Can't add more ports after module close")
requireIsChiselType(iodef, "io type")

// Clone the IO so we preserve immutability of data types
val iodefClone = try {
iodef.cloneTypeFull
} catch {
// For now this is going to be just a deprecation so we don't suddenly break everyone's code
case e: AutoClonetypeException =>
Builder.deprecated(e.getMessage, Some(s"${iodef.getClass}"))
iodef
}
module.bindIoInPlace(iodefClone)
iodefClone
}
}

/** Abstract base class for Modules, an instantiable organizational unit for RTL.
*/
// TODO: seal this?
Expand All @@ -99,6 +133,9 @@ abstract class BaseModule extends HasId {
//
protected var _closed = false

/** Internal check if a Module is closed */
private[core] def isClosed = _closed

// Fresh Namespace because in Firrtl, Modules namespaces are disjoint with the global namespace
private[core] val _namespace = Namespace.empty
private val _ids = ArrayBuffer[HasId]()
Expand Down Expand Up @@ -243,6 +280,8 @@ abstract class BaseModule extends HasId {
iodef.bind(PortBinding(this))
_ports += iodef
}
/** Private accessor for _bindIoInPlace */
private[core] def bindIoInPlace(iodef: Data): Unit = _bindIoInPlace(iodef)

/**
* This must wrap the datatype used to set the io field of any Module.
Expand All @@ -260,22 +299,7 @@ abstract class BaseModule extends HasId {
* TODO(twigg): Specifically walk the Data definition to call out which nodes
* are problematic.
*/
protected def IO[T<:Data](iodef: T): T = {
require(!_closed, "Can't add more ports after module close")
requireIsChiselType(iodef, "io type")

// Clone the IO so we preserve immutability of data types
val iodefClone = try {
iodef.cloneTypeFull
} catch {
// For now this is going to be just a deprecation so we don't suddenly break everyone's code
case e: AutoClonetypeException =>
Builder.deprecated(e.getMessage, Some(s"${iodef.getClass}"))
iodef
}
_bindIoInPlace(iodefClone)
iodefClone
}
protected def IO[T<:Data](iodef: T): T = chisel3.core.IO.apply(iodef)

//
// Internal Functions
Expand Down
30 changes: 26 additions & 4 deletions chiselFrontend/src/main/scala/chisel3/core/UserModule.scala
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,30 @@ abstract class UserModule(implicit moduleCompileOptions: CompileOptions)

val compileOptions = moduleCompileOptions

private[chisel3] def namePorts(names: HashMap[HasId, String]): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this detect naming conflicts? Previously we didn't need to worry about this because (presumably) you can't get a conflict with member accessors, but given arbitrary strings, I think this is where you want to error out where you have two wires fighting for the same name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently you can get conflicts using suggestName:

class TestModule extends Module {
  val io = IO(new Bundle { })
  val w = WireInit(10.U).suggestName("foo")
  val w2 = WireInit(10.U).suggestName("foo")
}

gives

circuit TestModule :
  module TestModule :
    input clock : Clock
    input reset : UInt<1>
    output io : {}

    wire foo : UInt
    foo <= UInt<4>("h0a")
    wire foo_1 : UInt
    foo_1 <= UInt<4>("h0a")

This PR doesn't change anything about that.

That being said, there definitely should be some tests illustrating how conflicts are resolved. TLDR, ports always win over things like wires, but _# is introduced when ports conflict as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to check for conflicts in namePorts and error out if one is detected? I don't know if it makes sense to refactor namespace to do that checking, but that's definitely out of scope for this,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think a stricter .Name API could error. I don't think we should change .suggestName to error since it's name suggests it won't, but maybe I can make it error just for ports? I'll look into how hard that is to do

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think it should error for ports. We don't have to worry about wires until suggestName is replaced, but I think that a particularly nasty case could be a conflict between a suggested name and a val name, and one of them (probably based on call order) will unexpectedly turn into the wrong thing in the generated verilog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It now errors for ports getting suggested (or otherwise named) to the same thing

for (port <- getModulePorts) {
port.suggestedName.orElse(names.get(port)) match {
case Some(name) =>
if (_namespace.contains(name)) {
Builder.error(s"""Unable to name port $port to "$name" in $this,""" +
" name is already taken by another port!")
}
port.setRef(ModuleIO(this, _namespace.name(name)))
case None => Builder.error(s"Unable to name port $port in $this, " +
"try making it a public field of the Module")
}
}
}


private[core] override def generateComponent(): Component = {
require(!_closed, "Can't generate module more than once")
_closed = true

val names = nameIds(classOf[UserModule])

// Ports get first naming priority, since they are part of a Module's IO spec
for (port <- getModulePorts) {
require(names.contains(port), s"Unable to name port $port in $this")
port.setRef(ModuleIO(this, _namespace.name(names(port))))
}
namePorts(names)

// Then everything else gets named
for ((node, name) <- names) {
Expand Down Expand Up @@ -170,6 +183,15 @@ abstract class LegacyModule(implicit moduleCompileOptions: CompileOptions)
names
}

private[chisel3] override def namePorts(names: HashMap[HasId, String]): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does LegacyModule really needs its own specialized code path? I think LegacyModule already checks for extraneous IOs as part of existing code, so suggestName-based IOs should already be rejected. In the case of a suggestName("io") conflict, hopefully the name conflict mechanism should cause it to error out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It probably doesn't, I'll look into it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually does, the regular UserModule flow involves checking suggestedName and LegacyModule needs to not do that. I added a test and then tried merging them but decided to keep them separate

for (port <- getModulePorts) {
// This shoudl already have been caught
if (!names.contains(port)) throwException(s"Unable to name port $port in $this")
val name = names(port)
port.setRef(ModuleIO(this, _namespace.name(name)))
}
}

private[core] override def generateComponent(): Component = {
_compatAutoWrapPorts() // pre-IO(...) compatibility hack

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ private[chisel3] trait HasId extends InstanceId {
for(hook <- postname_hooks) { hook(name) }
this
}
private[chisel3] def suggestedName: Option[String] = suggested_name
private[chisel3] def addPostnameHook(hook: String=>Unit): Unit = postname_hooks += hook

// Uses a namespace to convert suggestion into a true name
Expand Down
2 changes: 2 additions & 0 deletions src/main/scala/chisel3/package.scala
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,8 @@ package object chisel3 { // scalastyle:ignore package.object.name
type RawModule = chisel3.core.UserModule
type ExtModule = chisel3.core.ExtModule

val IO = chisel3.core.IO

// Implicit conversions for BlackBox Parameters
implicit def fromIntToIntParam(x: Int): IntParam = IntParam(BigInt(x))
implicit def fromLongToIntParam(x: Long): IntParam = IntParam(BigInt(x))
Expand Down
6 changes: 2 additions & 4 deletions src/test/scala/chiselTests/NamingAnnotationTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,14 @@ package chiselTests

import chisel3._
import chisel3.internal.InstanceId
import chisel3.experimental.{chiselName, dump}
import chisel3.experimental.{chiselName, dump, MultiIOModule}
import org.scalatest._
import org.scalatest.prop._
import chisel3.testers.BasicTester

import scala.collection.mutable.ListBuffer

trait NamedModuleTester extends Module {
val io = IO(new Bundle() {}) // Named module testers don't need IO

trait NamedModuleTester extends MultiIOModule {
val expectedNameMap = ListBuffer[(InstanceId, String)]()
val expectedModuleNameMap = ListBuffer[(Module, String)]()

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// See LICENSE for license details.

package chiselTests
package experimental

import chisel3._
import chisel3.experimental.MultiIOModule

// NOTE This is currently an experimental API and subject to change
// Example using a private port
class PrivatePort extends NamedModuleTester {
private val port = expectName(IO(Input(UInt(8.W))), "foo")
port.suggestName("foo")
}

// Example of using composition to add ports to a Module
class CompositionalPort(module: NamedModuleTester, name: String) {
import chisel3.experimental.IO
val foo = module.expectName(IO(Output(Bool())), name)
foo.suggestName(name)
foo := true.B
}

class CompositionalPortTester extends NamedModuleTester {
val a = new CompositionalPort(this, "cheese")
val b = new CompositionalPort(this, "tart")
}

class PortsWinTester extends NamedModuleTester {
val wire = expectName(Wire(UInt()), "wire_1")
val foo = expectName(Wire(UInt()).suggestName("wire"), "wire_2")
val output = expectName(IO(Output(UInt())).suggestName("wire"), "wire")
}

class ProgrammaticPortsSpec extends ChiselFlatSpec {

private def doTest(testMod: => NamedModuleTester): Unit = {
var module: NamedModuleTester = null
elaborate { module = testMod; module }
assert(module.getNameFailures() == Nil)
}

"Programmatic port creation" should "be supported" in {
doTest(new PrivatePort)
}

"Calling IO outside of a Module definition" should "be supported" in {
doTest(new CompositionalPortTester)
}

"Ports" should "always win over internal components in naming" in {
doTest(new PortsWinTester)
}

"LegacyModule" should "ignore suggestName on ports" in {
doTest(new Module with NamedModuleTester {
val io = IO(new Bundle {
val foo = Output(UInt(8.W))
})
expectName(io.suggestName("cheese"), "io")
expectName(clock.suggestName("tart"), "clock")
expectName(reset.suggestName("teser"), "reset")
})
}

"SuggestName collisions on ports" should "be illegal" in {
a [ChiselException] should be thrownBy {
elaborate(new MultiIOModule {
val foo = IO(UInt(8.W)).suggestName("apple")
val bar = IO(UInt(8.W)).suggestName("apple")
})
}
}
}