Implement asTypeOf, refactor internal APIs #450

ducky64 · 2017-01-24T02:21:19Z

Several refactor changes that shouldn't affect non-nonsensical Chisel:

Implement asTypeOf (resolution for Make reinterpret cast conversions (fromBits / asUInt / etc) consistent #433)
fromBits currently untouched. Will be chisel3-deprecated if possible as a future PR.
Refactor code that needs to create a super-type of several types into cloneSupertype (eliminates the premise of Is there a reason we do an explicit Mux compatibility check? #446)
- This should be the only thing that changes externally visible API, in that the checks are now more consistent. oneHotMux now checks against inconsistent input types.
Split out Mux (raised in Is there a reason we do an explicit Mux compatibility check? #446), and simply using above
Simplify Vec construction using above
Add a typeEquivalent internal API. This and cloneSupertype eliminates the need for typesCompatible, which is inexplicably a Mux operation.
Move cloneTypeWidth from Data to Bits. It used to silently drop the width parameter when called on any non-Bits, which is just sketchy. This makes uses of it explicit and always correct, minus the Bool case.
- Eventually want to get rid of cloneTypeWidth. The only stragglers are Bits.pad (which can instead create a new UInt/etc from scratch) and Reg (which clears widths behind the scenes for you). Not sure how to address the Reg case.
Deprecate flatten, hopefully will remove it eventually using more local operations. The stragglers are mainly Bindings directionality checks, which can be made more local with a resolution to Consolidate directionality data #440.

Originally the intention was to remove fromBits and the less clean APIs that it relied on (like flatten), but many more yaks were shaved. flatten hasn't been removed yet, but should be possible in the near future,

ducky64 · 2017-02-01T22:10:52Z

Aggregate.getElements now public.
Data.flatten is still private, since I don't think it's a good Data-level API or is well named (since it returns Element). It might make sense as a utils function or something.

jackkoenig · 2017-01-26T01:03:53Z

src/test/scala/chiselTests/ReinterpretCast.scala

+  assert( DataMirror.widthOf(truncate).get == 3 )
+  assert( truncate === 3.U )
+  assert( DataMirror.widthOf(expand).get == 3 )
+  assert( expand === 1.U )


Why DataMirror.widthOf instead of just .widthOption or .getWidth, also, why does DataMirror.widthOf exist? @sdtwigg

I think DataMirror provides a more reflection-like API for reflection-like operations.

In your opinion, in user Chisel code should people use DataMirror over .getWidth?

I think there's a good argument for an explicit reflection-like API, especially since there are subtleties with width (inferred width doesn't get computed until after Chisel).

But in any case we should standardize on one, and deprecate the other.

Fair enough, a discussion for a different PR I think

jackkoenig · 2017-01-26T01:08:58Z

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

@@ -15,26 +15,22 @@ import chisel3.internal.sourceinfo._
  * of) other Data objects.
  */
 sealed abstract class Aggregate extends Data {
-  private[core] def cloneTypeWidth(width: Width): this.type = cloneType
-  private[core] def width: Width = flatten.map(_.width).reduce(_ + _)
+  private[core] def getElements: Seq[Data]


As discussed at the Chisel meeting today, getElements could be public, no?

You might be reviewing an outdated version, getElements is public...

jackkoenig · 2017-02-13T18:33:36Z

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

@@ -15,26 +15,22 @@ import chisel3.internal.sourceinfo._
  * of) other Data objects.
  */
 sealed abstract class Aggregate extends Data {
-  private[core] def cloneTypeWidth(width: Width): this.type = cloneType
-  private[core] def width: Width = flatten.map(_.width).reduce(_ + _)
+  def getElements: Seq[Data]


Good catch, will do.

jackkoenig · 2017-02-15T00:00:44Z

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

-  private[chisel3] lazy val flatten = elements.toIndexedSeq.flatMap(_._2.flatten)
+  private[core] override def typeEquivalent(that: Data): Boolean = that match {
+    case that: Record =>
+      this.getClass == that.getClass &&


Is this actually what we want? Bulk connect works on things that are not the same class but have the same elements

I think there's potential debate to be had here. I think a stricter check makes sense, since I think there's an argument to be made for type equality having meaning.

I didn't realize in doAggregateMux we are currently requiring that the classes be the same so yeah, I agree with keeping this strict. It is interesting that you can't create a Mux between two otherwise identical anonymous Bundles but you can bulk connect them.

For Mux they kind of have to be the same class, because you need to pick one as the return type. The least arbitrary thing to do is to pick one but require it be the same as the other.

For connections, there is a debate to be had, but relaxing it later is always easier than imposing a new restriction.

I think they should require the same class. I can't currently think of a use case where you want to create the same Record in everything but class. For library-like interfaces I think you would want to end up using the library's Record constructor, rather than trying to create your own record with the same elements.

Aye, it all makes sense now!

jackkoenig · 2017-02-15T00:01:19Z

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

@@ -390,6 +392,9 @@ abstract trait Num[T <: Data] {
 sealed class UInt private[core] (width: Width, lit: Option[ULit] = None)
    extends Bits(width, lit) with Num[UInt] {

+  private[core] override def typeEquivalent(that: Data): Boolean =
+    that.isInstanceOf[UInt] && this.width == that.width  // TODO: should this be true for unspecified widths?


Yes I think it should be true for unspecified widths

jackkoenig · 2017-02-15T01:52:18Z

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

-  private[chisel3] lazy val flatten: IndexedSeq[Bits] =
-    (0 until length).flatMap(i => this.apply(i).flatten)
+  override def getElements: Seq[Data] =
+    (0 until length).map(apply(_))


I don't think there's a reason to re-define the ScalaDoc in override functions in subclasses.

fair enough

jackkoenig · 2017-02-15T01:54:55Z

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

-    })
-    res
+  private[core] override def connectFromBits(that: Bits)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions) {
+    this := that.asUInt
  }


Scala style is to not use procedure syntax: http://docs.scala-lang.org/style/declarations

ie. ): Unit = {

jackkoenig · 2017-02-15T01:57:07Z

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

+
+  def do_asTypeOf[T <: Data](that: T)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions): T = {
+    val thatCloned = Wire(that.chiselCloneType)
+    thatCloned.connectFromBits(asUInt())


this is just my opinion but I generally prefer to use this. when invoking methods on the object at hand. That asUInt() just looks like some magical functional call rather than calling something on this

jackkoenig · 2017-02-15T01:57:19Z

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

+
+  /** Assigns this node from Bits type. Internal implementation for asTypeOf.
+    */
+  private[core] def connectFromBits(that: Bits)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions)


Return type

jackkoenig · 2017-02-15T01:57:46Z

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

-    val ret = Wire(this.cloneType)
-    ret := that
-    ret.asInstanceOf[this.type]
+  private[core] override def connectFromBits(that: Bits)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions) {


This is same comment as the previous one about procedure syntax, I think there are a few of these in this PR

ducky64 · 2017-02-15T04:40:38Z

Everything else fixed. More discussion on role if type in typeEquivalent may be necessary

jackkoenig · 2017-02-15T19:45:14Z

There's a conflict in Bits.scala, think you can resolve it?

ducky64 · 2017-02-15T22:05:23Z

Conflict fixed.

CloneSupertype now works for fixed point (requiring the binary point to be the same) and parts of Analog have been updated to fit the new API.

Merging as soon as tests go green...

ducky64 changed the title ~~Implement asTypeOf, refactor some other APIs~~ Implement asTypeOf, refactor internal APIs Jan 24, 2017

ducky64 mentioned this pull request Jan 25, 2017

Deprecate old Reg with nulls constructor #455

Merged

ducky64 requested a review from jackkoenig January 28, 2017 07:27

jackkoenig requested changes Feb 15, 2017

View reviewed changes

ducky64 force-pushed the nukeBits branch from 4393eb3 to aecef26 Compare February 15, 2017 19:50

jackkoenig approved these changes Feb 15, 2017

View reviewed changes

ducky64 added 3 commits February 15, 2017 12:03

merge analog

29be3eb

Fix?

61cb20c

Fix

94c9b23

ducky64 force-pushed the nukeBits branch from aecef26 to 94c9b23 Compare February 15, 2017 21:49

actually add mux

5d2c231

ducky64 merged commit 375e2b6 into master Feb 15, 2017

ducky64 deleted the nukeBits branch February 15, 2017 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement asTypeOf, refactor internal APIs #450

Implement asTypeOf, refactor internal APIs #450

ducky64 commented Jan 24, 2017 •

edited

Loading

ducky64 commented Feb 1, 2017

jackkoenig Jan 26, 2017

ducky64 Feb 15, 2017

jackkoenig Feb 15, 2017

ducky64 Feb 15, 2017

jackkoenig Feb 15, 2017

jackkoenig Jan 26, 2017

ducky64 Feb 15, 2017

jackkoenig Feb 13, 2017

ducky64 Feb 15, 2017

jackkoenig Feb 15, 2017

ducky64 Feb 15, 2017

jackkoenig Feb 15, 2017

aswaterman Feb 15, 2017

colinschmidt Feb 15, 2017

jackkoenig Feb 15, 2017

jackkoenig Feb 15, 2017

jackkoenig Feb 15, 2017

ducky64 Feb 15, 2017

jackkoenig Feb 15, 2017

jackkoenig Feb 15, 2017

jackkoenig Feb 15, 2017

jackkoenig Feb 15, 2017

jackkoenig Feb 15, 2017

ducky64 commented Feb 15, 2017

jackkoenig commented Feb 15, 2017

ducky64 commented Feb 15, 2017

Implement asTypeOf, refactor internal APIs #450

Implement asTypeOf, refactor internal APIs #450

Conversation

ducky64 commented Jan 24, 2017 • edited Loading

ducky64 commented Feb 1, 2017

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

ducky64 commented Feb 15, 2017

jackkoenig commented Feb 15, 2017

ducky64 commented Feb 15, 2017

ducky64 commented Jan 24, 2017 •

edited

Loading