[RFC] [Proposal] Programmatic Port Creation #833

jackkoenig · 2018-06-09T00:22:47Z

The basic idea here is that people want to be able to create ports and name them without being required to assign the port to a val and let reflection name it.

The proposed API is that in RawModules and MultiIOModules, ports can be named in the normal way, but now they also support .suggestName. If no name is suggested, regular reflection will be used to try to name the port. If no name can be found by regular reflection, then it will error because we don't want _T_# port names. Ports are also not required to be assigned to some class field, they can be used however the user wants, they just must be wrapped in IO.

LegacyModule remains unchanged, although .suggestName becomes an error if you try it on clock, reset, or io in a LegacyModule.

Type of change: other enhancement

Impact: API addition (no impact on existing code)

Development Phase: proposal/implementation

Release Notes

Allowed in RawModule and MultIOModule

ducky64 · 2018-06-09T01:08:04Z

This seems reasonable. I'd like to see wider consensus on a naming API, since I don't think we've formalized on one after @chiselName. I don't quite like suggestName.

Potentially related: issue #666, where string interpolation of IO names (or generally, parameterized IO names) was a requested feature. setName was also thrown around here, but that also sounds more imperative/mutable than declarative/immutable.

One major issue: if IOs aren't a val, they aren't externally accessible by member access, until getAllIoAsRecord (or similar) happens. But even then, you'd still be dealing with stringly-typed fields, so that doesn't seem to be a good fit. The main use seems to be generating Modules that need to conform to an external API (for example, Chisel blocks that will be instantiated by Verilog)? It might also be useful for generating BlackBox/ExtModule interfaces, but you would also need to access the IOs from Chisel, which isn't possible if they aren't vals?

terpstra · 2018-06-14T13:58:10Z

So the use-case where I want to use this is:

class HelperThing(index: Int) {
  val foo = IO(new Bool)
  foo.suggestName(s"foo_${index}")
  foo := true.B
}

class Top extends Module {
  val helper = new HelperThing(42)
}

The real scenario where I want to use this is obviously part of a much larger design. The intent is to be able to factor the complexity of a very large Module. We've done this in the past with traits, but following the OOP mantra of "Composition over inheritance", the above approach scales better. In particular, it let's us build 'HelperThing' itself out of re-usable bits of code and you can instantiate it more than once in Top.

As you can see, there is no concern about losing connectivity, because you always ask for top.helper.foo. It's still accessible, just not as a direct member variable of the module.

colinschmidt · 2018-06-14T15:43:46Z

@ducky64 Is the concern you have with suggestName that the name is not good, or that you would prefer a less imperative API?

I think this feature makes perfect sense, and its on the user to avoid losing access to their IOs.

chick

This looks ok to me. Is there any documentation in the wiki or anywhere else

terpstra · 2018-06-14T18:08:27Z

This PR still needs to make 'IO' some global object, not a protected member variable of Module. Otherwise, it will still be a total PITA to use.

jackkoenig · 2018-06-14T18:09:02Z

Sorry yeah, was meaning to add that today

terpstra · 2018-06-14T18:31:44Z

Regarding the 'suggestName', I don't know if you guys have seen the 'ValName' API used in rocket-chip. The 'IO' function seems a perfect candidate to use that API. You want to require it to be bound to a val anyway. The name also doesn't need to be passed deeply to helper methods.

I think ValName variants of 'Reg', 'Wire', and 'IO' would have been a better way to go than @chiselName.

ducky64 · 2018-06-14T19:12:08Z

I understand the use case now - though it might make sense for this to be an experimental feature if possible (much like with MultiIOModule)? It makes sense in theory, but its unclear if there will be issues in practice?

I would also prefer naming to be as invisible to the user as possible, keeping with the current style. Can we use reflection to recurse into objects? Also, should the generated name be fully path qualified (and closer to how you would access it from Scala), and is that possible?

terpstra · 2018-06-14T19:18:06Z

For the place where I need this functionality, having to make an explicit suggestName call is totally acceptable. Attempting to use recursive reflection in my situation is also unlikely to work, because the object relationship is not quite as simple as I presented it.

jackkoenig · 2018-06-14T19:55:10Z

I definitely eventually want to have ValName variants on Reg, Wire, etc. and possibly even operations like +. It's a pain to add it everywhere though. For Chisel codebase reasons, maybe we could make a def macro that does all the boiler plate of final def blah = macro do_blah...

Because @chiselName adds .suggestName to everything, the stricter behavior just breaks the annotation on all LegacyModules

ducky64 · 2018-06-14T21:34:22Z

With regards to method naming (hello, bikeshedding): I don't like suggestName because it seems more to be a naming hint, and conveys that the system can discard user input (which would seem to be an antipattern). Something that's more of "use this name, or crash" would make more sense to me. And something more declarative would be nice, keeping in line with the rest of the chisel API (we don't write makeReg(...), we just write Reg(...)).

I think it's worth thinking about whether this could fit into any of the existing naming systems, just to reduce the learning curve. Currently, it seems that we have basic chisel (which we've put a lot of effort into reducing the learning curve and providing tutorials), and then you have rocket-chip with the learning curve of a high brick wall, because it uses a bunch of really advanced features. Yes, I know they're needed for a reason, but I'd like to see if there's a way to start unifying things.

ducky64 · 2018-06-14T21:36:37Z

ValName: would be interesting, though I thought that Scala wanted to clamp down on macros' ability to inspect outer scopes?
We could certainly write macros to write macros, though there are a few plumbing issues that we would need to discuss (how to deal with inheritance overrides, how to specify the implicit parameters without new people looking at the code and immediately going 'wtf').

Major changes, re-review requested

jackkoenig · 2018-06-14T22:03:37Z

I've added IO as an experimental API that can be called outside of Module definitions.

I also prototyped a different way of implementing the package aliasing, I think the val type thing is kind of weird in the ScalaDoc, so this presents an alternative way where the scaladoc for object IO will actually be in package chisel3.experimental, thoughts? If people like this I think we should refactor the entire API and make things in chisel3.core private wherever possible

terpstra · 2018-06-14T23:39:07Z

I've just tried using this PR in my work-in-progress branch in fpga-shells. You can see how nicely it has cleaned up the VC707Shell Module in sifive/fpga-shells@e2c1d97

The 'Overlays' are the pieces I add together to make the Shell.

Ultimately, since suggestName is an already existing API, that works on wires (and was just ignored on IOs), sticking with suggestName is the least invasive form of naming in this context. Perhaps a better API for the name could be something in a follow-up PR which also tackles the larger naming issue?

ducky64 · 2018-06-15T21:35:30Z

Notes from today's meeting discussions:

People want this merged ASAP.
My main concern with naming is that it will break user code if we change it. I'm happy with coming up with a new name now, implementing using that, but deferring the stricter checking implementation until the next milestone release.
- Basically decide on the stuff we can't be agile about right now, while worry about the lower-impact changes later - on the assumption that the power users who are going to be using an experimental API on a non-milestone release will probably be following PRs and understanding limitations.
Strawman naming APIs (let's come to a consensus on one?):
- IO(example).Name(nameString, IO(example).name(nameString)
- Name(IO(example), nameString), Named(IO(example), nameString)
Another variant proposed during the meeting today: IO(example)(nameString) (second argument list is optional, something like ValNamer could automatically populate that unless the user overrides it)

Initially, ValName is experimentally only used by the experimental version of IO. It provides both the ability to get names from vals without reflection, while also providing a way to set the name explicitly.

jackkoenig · 2018-06-15T23:19:48Z

I've borrowed ValName from rocket-chip and illustrated how it could be used with the experimental IO. Currently, it looks like: IO(example)(ValName(nameString)), just as a starting proposal.

jackkoenig · 2018-06-16T00:04:38Z

One potential issue with ValName is it doesn't look like it can be explicitly set in the system with macros adding the implicits (as done on UInt operators for example) in one line at least. If we added to those operators one would have to write:

val x = {
  implicit val y = ValName("hi")
  18.U + 13.U
}

As far as I can tell, it's not possible to pass the macro added implicits explicitly (which I think is a feature with SourceInfo and CompileOptions.

This doesn't mean it wouldn't still be incredibly useful, but perhaps an API like (18.U + 13.U).Name("hi") still makes sense

ducky64 · 2018-06-16T18:59:49Z

Wait, why can't the implicits be passed explicitly?

I think that for a user-facing API, something more explicit like (18.U + 13.U).Name("hi") (compared to an implicit parameter list) would make more sense. Is it also possible for def macros to insert that (or were there significant restrictions on where you could modify the tree)?

I also don't think it's a great idea to have the implicit parameter list explode. We currently have SourceInfo and CompileOptions - neither of which users should specify manually, and it might make sense to be consistent in that none of the implicit parameters should (or could?) be manually specified.

jackkoenig · 2018-06-18T18:37:07Z

What I mean is I don't think it's possible to manually pass implicits that are added by a def macro (for example, on UInt.+). The whole point of the macro is that an additional parameter list is interpreted as calling .apply so it necessarily prevents passing the implicits explicitly.

jackkoenig · 2018-06-18T18:42:32Z

I think ValName is beyond the scope of this PR so I'm going to remove it.

I also agree with @terpstra that coming up with a replacement for suggestName is a bit beyond the scope too--suggestName is already a public API that is in use. Since this is an experimental API, changing it in the future to require something other than suggestName isn't unreasonable.

ducky64 · 2018-06-18T18:45:28Z

Sure, renaming the naming APIs can be its own issue. Possibly combined with ValName. As long as everyone using this is fine with getting deprecation warnings or (less likely, because suggestName is already in use) broken code when naming gets fixed.

…tName" This reverts commit 6ec7c7d.

terpstra · 2018-06-18T20:12:42Z

I'm totally fine with using an experimental API that may disappear later. All I care about is being able to declare ports programmatically, somehow. The place where I'd use this is nicely abstracted into a shared piece of code anyway.

ducky64

Mostly looks fine, but I think there's some implementation details that might warrant a bit more attention? In particular, name conflicts, which I think namespace resolves by appending digits. Might also want a testcase to make the conflict behavior explicit.

ducky64 · 2018-06-18T20:38:35Z

src/test/scala/chiselTests/experimental/ProgrammaticPortsSpec.scala

+  port.suggestName("foo")
+}
+
+class PortAdder(module: NamedModuleTester, name: String) {


Can you add a comment here indicating the class compositional pattern (I initially interpreted this as a roundabout way to have shared functions)

ducky64 · 2018-06-18T20:39:29Z

chiselFrontend/src/main/scala/chisel3/core/Module.scala

@@ -79,6 +79,42 @@ object Module {
  def currentModule: Option[BaseModule] = Builder.currentModule
 }

+private[chisel3] trait IOImpl {


Does this need to be a trait, or can this just be an object? It doesn't appear you're extending the trait anywhere, and you should be able to forward vals in the package object?

The trait is extended by a private object IO used internally to chisel3.core and by an object in package chisel3.experimental (in package.scala). I was kind of prototyping a new way of doing the val forwarding where the in the ScalaDoc the actual documentation exists where you expect it to instead of having to go look in chisel3.core. I grant that perhaps this PR isn't the appropriate place to try out changing how the documentation exists in the ScalaDoc so I'll switch to the existing pattern.

ducky64 · 2018-06-18T20:40:56Z

chiselFrontend/src/main/scala/chisel3/core/UserModule.scala

@@ -38,17 +38,22 @@ abstract class UserModule(implicit moduleCompileOptions: CompileOptions)

  val compileOptions = moduleCompileOptions

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


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

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.

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,

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

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.

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

ducky64 · 2018-06-18T20:42:25Z

chiselFrontend/src/main/scala/chisel3/core/UserModule.scala

@@ -170,6 +175,13 @@ abstract class LegacyModule(implicit moduleCompileOptions: CompileOptions)
    names
  }

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


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?

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

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

jackkoenig · 2018-06-19T23:34:14Z

@ducky64 I added some clarifying tests and incorporated some of your feedback!

ducky64

lgtm

I see why suggestName needs to be ignored for LegacyModule instead of relying on conflict detection.

jackkoenig added 3 commits June 8, 2018 11:16

Add tests for suggestName

82fe01d

Allow suggestName and programmatic port creation

cce3378

Allowed in RawModule and MultIOModule

Add update to test infrastructure needed for new tests

92e2d95

jackkoenig requested review from chick, ducky64, terpstra and ucbjrl June 9, 2018 00:22

chick previously approved these changes Jun 14, 2018

View reviewed changes

Revert LegacyModule behavior to do nothing when port has suggestName

7ce2562

Because @chiselName adds .suggestName to everything, the stricter behavior just breaks the annotation on all LegacyModules

Make IO [experimentally] callable from outside a Module definition

cbb25e8

terpstra approved these changes Jun 14, 2018

View reviewed changes

Add ValName macro from rocket-chip, favor ValName over suggestName

6ec7c7d

Initially, ValName is experimentally only used by the experimental version of IO. It provides both the ability to get names from vals without reflection, while also providing a way to set the name explicitly.

jackkoenig added 3 commits June 18, 2018 11:47

Revert "Add ValName macro from rocket-chip, favor ValName over sugges…

24ef51c

…tName" This reverts commit 6ec7c7d.

Rename and remove suggestName tests in favor of ProgrammaticPortsSpec

64ddbab

Merge branch 'master' into prog-ports

0b28183

ducky64 requested changes Jun 18, 2018

View reviewed changes

Change IO to object, catch multiple ports named same thing, add tests

fa7e1ab

ducky64 approved these changes Jun 20, 2018

View reviewed changes

jackkoenig added this to the 3.2.0 milestone Jun 20, 2018

Fix typo

10e9227

jackkoenig merged commit 980778b into master Jun 21, 2018

jackkoenig deleted the prog-ports branch June 21, 2018 00:09

		@@ -38,17 +38,22 @@ abstract class UserModule(implicit moduleCompileOptions: CompileOptions)

		val compileOptions = moduleCompileOptions

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

[RFC] [Proposal] Programmatic Port Creation #833

[RFC] [Proposal] Programmatic Port Creation #833

Conversation

jackkoenig commented Jun 9, 2018

ducky64 commented Jun 9, 2018

terpstra commented Jun 14, 2018 • edited Loading

colinschmidt commented Jun 14, 2018

chick left a comment

Choose a reason for hiding this comment

terpstra commented Jun 14, 2018

jackkoenig commented Jun 14, 2018

terpstra commented Jun 14, 2018

ducky64 commented Jun 14, 2018

terpstra commented Jun 14, 2018

jackkoenig commented Jun 14, 2018

ducky64 commented Jun 14, 2018

ducky64 commented Jun 14, 2018

jackkoenig commented Jun 14, 2018

terpstra commented Jun 14, 2018

ducky64 commented Jun 15, 2018

jackkoenig commented Jun 15, 2018

jackkoenig commented Jun 16, 2018

ducky64 commented Jun 16, 2018

jackkoenig commented Jun 18, 2018

jackkoenig commented Jun 18, 2018

ducky64 commented Jun 18, 2018

terpstra commented Jun 18, 2018

ducky64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackkoenig commented Jun 19, 2018

ducky64 left a comment

Choose a reason for hiding this comment

terpstra commented Jun 14, 2018 •

edited

Loading