Directions internals mega-refactor #617

ducky64 · 2017-05-25T02:21:34Z

Part 1 of mega-change in #578

Major notes:

Input(...) and Output(...) now (effectively) recursively override their elements' directions
Nodes given userDirection (Input, Output, Flip - what the user assigned to that node) and actualDirection (Input, Output, None, but also Bidirectional and BidirectionalFlip for mostly Aggregates), because of the above (since a higher-level Input(...) can override the locally specified user direction).
DataMirror (node reflection APIs) added to chisel3.experimental. This provides ways to query the user given direction of a node as well as the actual direction.
checkSynthesizable replaced with requireIsHardware and requireIsChiselType and made available in chisel3.experimental.

Internal changes notes:

toType moved into Emitter, this makes the implementation cleaner especially considering that Vec types can't be flipped in FIRRTL. This also more clearly separates Chisel frontend from FIRRTL emission.
Direction separated from Bindings, both are now fields in Data, and all nodes are given hierarchical directions (Aggregates may be Bidirectional). The actualDirection at the Element (leaf) level should be the same as binding directions previously.
Bindings are hierarchical, children (of a, for example, Bundle) have a ChildBinding that points to their parent. This is different than the previous scheme where Bindings only applied at the Element (leaf) level.
Lots of small misc clean up.

Discussion points

Naming of requireIsHardware and requireIsChiselType
This separates the Input(...), Output(...), Flipped(...) from the directions, and also (by necessity) splits userDirection and actualDirection. I think this is cleaner, but thoughts?
requireIsHardware is currently a top-level method, as opposed to (myChiselData).requireIsHardware. Thoughts? I think getType (future PR) will be in a similar style.
~~Input(...), Output(...), Flipped(...) have been changed not to clone, for efficiency reasons since the usual use is in inline with Bundles. Thoughts?~~ They clone again now, not cloning seems to break a lot of code.

Future PRs will address other parts of #578, including stricter direction checks that aren't a side-effect of this internal refactor, stricter checks and splitting of binding operations (Wire vs. WireInit), and node operations not introduced here (getType and deprecation of chiselCloneType). Since those shouldn't mess with internals, those should be much smaller.

Testing with rocket-chip pending.

ducky64 · 2017-05-26T01:32:09Z

rocket-chip builds on this now, with one necessary change:

in (rocket-chip)/src/main/scala/jtag/JtagTap.scala
val TCK = Clock(OUTPUT)
must be changed to the new style (since JTAG is chisel3._)
val TCK = Output(Clock())

jackkoenig · 2017-06-02T21:35:01Z

For the record, I ran this branch with rocket-chip and it passes Travis: chipsalliance/rocket-chip#770

jackkoenig

Overall it looks good. I have a collection of primarily nitpicks and documentation questions. Great work!

jackkoenig · 2017-05-31T18:12:57Z

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

@@ -68,25 +114,21 @@ object Vec {

    // Check that types are homogeneous.  Width mismatch for Elements is safe.
    require(!elts.isEmpty)
+    elts foreach {requireIsHardware(_, "vec element")}


should always use . notation with foreach

jackkoenig · 2017-06-01T17:44:16Z

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

+      if (!msg.isEmpty()) {
+        throw new Binding.BindingException(s"$msg '$node' must be hardware, not a bare Chisel type")
+      } else {
+        throw new Binding.BindingException(s"'$node' must be hardware, not a bare Chisel type")


It's not a big deal, but in my opinion, this check for if msg is empty couple be better written as:

val prefix = if (msg.nonEmpty) s"$msg " else "" throw new Binding.BindingException(s"$prefix'$node' must be hardware, not a bare Chisel type")

agree, changed

jackkoenig · 2017-06-01T18:09:45Z

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

+object requireIsChiselType {
+  def apply(node: Data, msg: String = "") = if (node.hasBinding) {
+    if (!msg.isEmpty()) {
+      throw new Binding.BindingException(s"$msg '$node' must be a Chisel type, not hardware")


I know it was like this before, but I personally would prefer using different types for the different sub BindingExceptions, then we could match on the specific exception thrown in testing.

jackkoenig · 2017-06-01T18:11:22Z

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

+      case PortBinding(_) => direction match {
+        case ActualDirection.Output => Output
+        case ActualDirection.Input => Input
+        case dir => throw new RuntimeException(s"Unexpected port element direction '$dir'")


Why is this a RunTimeException and not a ChiselException or something else?

This is more of a Chisel internal fault rather than something the user did. The idea here is to give a slightly more informative message than the standard "nothing matched" error, but Scala doesn't seem to have a built-in facility for specifying that.

I see, we should probably standardize with a ChiselInternalException or something but this is fine for now

jackkoenig · 2017-06-02T00:43:12Z

src/test/scala/chiselTests/RecordSpec.scala

@@ -24,7 +24,8 @@ trait RecordSpecUtils {
    override def cloneType = (new MyBundle).asInstanceOf[this.type]
  }
  // Useful for constructing types from CustomBundle
-  val fooBarType = new CustomBundle("foo" -> UInt(32.W), "bar" -> UInt(32.W))
+  // This is a def because each call to this needs to return a new instance


Hilariously, my original comment implies it should be a def, I guess I just goofed.

jackkoenig · 2017-06-06T00:10:38Z

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

 }
+// Top-level binding representing hardware, not a pointer to another binding
+sealed trait TopBinding extends Binding


By "top" here do you mean just a bound type inside of a module? Children are fields in an aggregate? Top kind of misleads me here since we usually refer to "Top" as the top of the circuit but I'm not sure what a better word would be....

I guess that's one way to interpret it.
To me, it means basically "not ChildBinding", and Top means the top of a potential Aggregate hierarchy. I've clarified the comment a bit.

jackkoenig · 2017-06-06T00:20:41Z

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

+        // For bidirectional, must issue a bulk connect so subelements are resolved correctly.
+        // Bulk connecting two wires may not succeed because Chisel frontend does not infer
+        // directions.
+        (vec zip elts) foreach {x => x._1 <> x._2}


dots for these foreaches as well. Also you generally are supposed to put spaces on each side of curly braces (although you could just use parentheses here)

fixed, and changed to parens

jackkoenig · 2017-06-06T00:22:11Z

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

    }
+    // TODO port technically isn't directly child of this data structure, but the result of some
+    // muxes / demuxes. However, this does make access consistent with the top-level bindings.
+    // Perhaps there's a cleaner way of accomlpishing this...


accomplishing

fixed.

And I had the red squigglies in my IDE too...

jackkoenig · 2017-06-06T00:22:42Z

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

+    // TODO port technically isn't directly child of this data structure, but the result of some
+    // muxes / demuxes. However, this does make access consistent with the top-level bindings.
+    // Perhaps there's a cleaner way of accomlpishing this...
+    port.bind(ChildBinding(this), reconstructedResolvedDirection)


I see the comment but I don't really get why this is a ChildBinding

This has to take the properties of (and act like) its top-level binding, but really isn't (it's more of an accessor, really).

For example, an accessor into a Reg(Vec(...)) behaves like a Reg, but isn't a top-level Reg. I'm not sure how this should be resolved cleanly.

jackkoenig · 2017-06-06T00:23:10Z

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

+      case ActualDirection.Input => UserDirection.Input
+      case ActualDirection.Output => UserDirection.Output
+      case ActualDirection.Bidirectional | ActualDirection.Unspecified => UserDirection.Unspecified
+      case ActualDirection.BidirectionalFlip => UserDirection.Flip


Is this match the only reason why there is both Bidirectional and BidirectionalFlip?

might it not be better to just store the UserDirection rather than muddying up the ActualDirections if so?

The reason for having BidirectionalFlip as an ActualDirection is to potentially emitting Bundle bulk-connects to FIRRTL instead of resolving everything to leaves. This makes the check easier, as it can happen at the top level instead of resolving down to leaves.

I guess what I'm getting at is that this seems like a super easy way to have bugs when dealing with directions because you have to remember that there are actually two bidirectional ones. What if Bidirectional were a case class instead with a field to indicated "flippedness" so that you could match on:

case ActualDirection.Bidirectional(_) =>

instead of

case ActualDirection.Bidirectional | ActualDirection.BidirectionalFlip =>

and for cases where flippedness matters:

case ActualDirection.Bidirectional(Flipped) => case ActualDirection.Bidirectional(Default) =>

or something

Done.

Renamed ActualDirection.Bidirectional to ActualDirection.BidirectionalDefault
Created ActualDirection.Bidirectional object that can be matched against (like case ActualDirection.Bidirectional(_)). It takes an argument of type ActualDirection.Bidirection.Direction, so actually filling in the match is a namespace nightmare and I only expect it to be used with _. The argument is left in both as a reminder that this can match both default and flipped versions, and allowing extensibility for future versions that might relax the namespace issue.

jackkoenig · 2017-06-19T21:07:18Z

We need to check if this addresses #282

jackkoenig · 2017-06-19T21:14:20Z

Check if this fixes #588

ducky64

All comments should be addressed.
There should be a test to test for #588, but #282 is going to be a bigger problem.

ducky64 · 2017-06-19T22:00:02Z

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

@@ -68,25 +114,21 @@ object Vec {

    // Check that types are homogeneous.  Width mismatch for Elements is safe.
    require(!elts.isEmpty)
+    elts foreach {requireIsHardware(_, "vec element")}


ducky64 · 2017-06-19T22:02:49Z

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

+        // For bidirectional, must issue a bulk connect so subelements are resolved correctly.
+        // Bulk connecting two wires may not succeed because Chisel frontend does not infer
+        // directions.
+        (vec zip elts) foreach {x => x._1 <> x._2}


fixed, and changed to parens

ducky64 · 2017-06-19T22:05:24Z

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

+      case ActualDirection.Input => UserDirection.Input
+      case ActualDirection.Output => UserDirection.Output
+      case ActualDirection.Bidirectional | ActualDirection.Unspecified => UserDirection.Unspecified
+      case ActualDirection.BidirectionalFlip => UserDirection.Flip


The reason for having BidirectionalFlip as an ActualDirection is to potentially emitting Bundle bulk-connects to FIRRTL instead of resolving everything to leaves. This makes the check easier, as it can happen at the top level instead of resolving down to leaves.

ducky64 · 2017-06-19T22:05:56Z

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

    }
+    // TODO port technically isn't directly child of this data structure, but the result of some
+    // muxes / demuxes. However, this does make access consistent with the top-level bindings.
+    // Perhaps there's a cleaner way of accomlpishing this...


fixed.

And I had the red squigglies in my IDE too...

ducky64 · 2017-06-19T22:09:33Z

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

+    // TODO port technically isn't directly child of this data structure, but the result of some
+    // muxes / demuxes. However, this does make access consistent with the top-level bindings.
+    // Perhaps there's a cleaner way of accomlpishing this...
+    port.bind(ChildBinding(this), reconstructedResolvedDirection)


This has to take the properties of (and act like) its top-level binding, but really isn't (it's more of an accessor, really).

For example, an accessor into a Reg(Vec(...)) behaves like a Reg, but isn't a top-level Reg. I'm not sure how this should be resolved cleanly.

ducky64 · 2017-06-19T23:02:33Z

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

+    // This recursively walks the tree, and assigns directions if no explicit
+    // direction given by upper-levels (override Input / Output) AND element is
+    // directly inside a compatibility Bundle determined by compile options.
+    def assignCompatDir(data: Data, insideCompat: Boolean) {


fixed.
Is this something we can catch with scalastyle?

ducky64 · 2017-06-19T23:03:13Z

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

+    // directly inside a compatibility Bundle determined by compile options.
+    def assignCompatDir(data: Data, insideCompat: Boolean) {
+      data match {
+        case data: Element if (insideCompat) => data._assignCompatibilityExplicitDirection


ducky64 · 2017-06-19T23:04:30Z

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

+      data match {
+        case data: Element if (insideCompat) => data._assignCompatibilityExplicitDirection
+        case data: Element => // Not inside a compatibility Bundle, nothing to be done
+        case data: Aggregate => DataMirror.userDirectionOf(data) match {


ducky64 · 2017-06-19T23:05:09Z

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

+          case UserDirection.Unspecified | UserDirection.Flip => data match {
+            case data: Record if (!data.compileOptions.dontAssumeDirectionality) =>
+              data.getElements foreach (assignCompatDir(_, true))
+            case _ => data.getElements foreach (assignCompatDir(_, false))


Fixed.
Is this also something we can add as a scalastyle rule?

ducky64 · 2017-06-19T23:06:07Z

src/main/scala/chisel3/internal/firrtl/Emitter.scala

+        s"output ${e.id.getRef.name} : ${emitType(e.id)}"
+      case UserDirection.Flip | UserDirection.Input =>
+        s"input ${e.id.getRef.name} : ${emitType(e.id)}"
+    }


good point, fixed

jackkoenig · 2017-06-26T23:11:03Z

The new Bidirectional implementation has the problem of not correctly detecting when you don't exhaustively match on the different subtypes of ActualDirection. For example:

  DataMirror.directionOf(io) match {
    case Direction.Bidirectional(_) => println("Bidirectional")
  }

This does not give you a warning but should because it only actually handles bidirectional directions.

ducky64 · 2017-06-26T23:43:36Z

Seems like Scala can't properly generate an exhaustive match warning with an unapply, even if I change the type signature of the unapply target to be more specific (for example, having BidirectionalDefault and BidirectionalFlip extend abstract Bidirectional which extends ActualDirection).

It is possible to match on classes (for example, Bidirectional in the example above), but the syntax is different.

jackkoenig · 2017-06-27T00:03:36Z

The exhaustive match warning would work if Bidirectional were a case class with just flip as a subfield

ducky64 · 2017-06-27T00:31:45Z

So that does work.
But it also requires the user of the case class constructor and the direction specification, which means tons and tons of namespace qualifier fun.
Alternatives would be to eliminate the namespace qualification (but that pollutes the global namespace with things like Default and Flipped which are only used here in very specific cases) or to use a bool flipped (but that's not very intuitive and can be a source of bugs).

jackkoenig · 2017-06-27T01:26:59Z

In my opinion, this way is better since users of the public API probably won't usually care if it's flipped or not so will use Direction.Bidirectional(_). Anyway, thanks for all your hard work! Looks good!

ducky64 requested review from jackkoenig and sdtwigg May 25, 2017 02:21

ducky64 added 19 commits May 26, 2017 11:41

Refactor in progress, broken broken broken

36b2b2a

errors slowly going down...

fe0371b

Continued binding refactoring

7b118dc

Refactoring continued

fb08b48

Lazy evaluation of direction

4652a6f

checkpoint, everything still broken =(

956e5da

CFE compiles

14314d9

things almost working

d44caf5

stuffs

ba44039

whackamole

5ccfec9

whackamole continued

f4a1c40

more whackamole

c323e5c

Fixes and breakages

90bca09

Fix all the things, other than one compatibility bug =(

7bfd047

Compatibility bundle work

b99daf8

Cleanup

0dc5ce4

Recursive direction clobber test

d61434b

Compatibility fixes to make rocket build

4d5b464

allow direciton reassign

8bb40f3

ducky64 force-pushed the dirrefactor branch from b98b2f4 to 8bb40f3 Compare May 26, 2017 19:18

jackkoenig mentioned this pull request May 30, 2017

Bump Chisel and Firrtl chipsalliance/rocket-chip#770

Closed

Merge branch 'master' into dirrefactor

0e9557d

jackkoenig added a commit to chipsalliance/rocket-chip that referenced this pull request May 30, 2017

Try with chipsalliance/chisel#617

f410d15

Merge branch 'master' into dirrefactor

e1ad2b5

jackkoenig added a commit to chipsalliance/rocket-chip that referenced this pull request Jun 1, 2017

Try with chipsalliance/chisel#617

5dc5e7b

jackkoenig requested changes Jun 6, 2017

View reviewed changes

jackkoenig added this to the 3.0.0 milestone Jun 19, 2017

Address comments

9104478

ducky64 commented Jun 19, 2017

View reviewed changes

Better bidir matching

e9debe3

Change to Bidir case class

820a299

jackkoenig approved these changes Jun 27, 2017

View reviewed changes

Merge branch 'master' into dirrefactor

fbf087a

ducky64 merged commit aaee58d into master Jun 27, 2017

ducky64 deleted the dirrefactor branch June 27, 2017 01:35

ducky64 mentioned this pull request Jun 29, 2017

Improve error message when forgetting to use cloneType #636

Closed

This was referenced Jul 24, 2017

Recursively SetFirrtlDirection for Aggregate types #588

Closed

Consolidate directionality data #440

Closed

ducky64 mentioned this pull request Sep 25, 2017

UInt(width=...) constructor broken instead of deprecated? #691

Closed

jackkoenig mentioned this pull request Sep 28, 2017

Clone directions for compatibility code in chiselCloneType #629

Closed

jackkoenig mentioned this pull request Nov 27, 2019

Fix bidirectional Wire with Analog #1252

Merged

Directions internals mega-refactor #617

Directions internals mega-refactor #617

Conversation

ducky64 commented May 25, 2017 • edited Loading

ducky64 commented May 26, 2017 • edited Loading

jackkoenig commented Jun 2, 2017

jackkoenig 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

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, 2017

jackkoenig commented Jun 19, 2017

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

jackkoenig commented Jun 26, 2017

ducky64 commented Jun 26, 2017

jackkoenig commented Jun 27, 2017

ducky64 commented Jun 27, 2017

jackkoenig commented Jun 27, 2017

ducky64 commented May 25, 2017 •

edited

Loading

ducky64 commented May 26, 2017 •

edited

Loading