Bundle Literals Framework #820

ducky64 · 2018-05-11T19:45:33Z

Related issue: #805

Type of change: other enhancement

Impact: API addition, API deprecation

Development Phase: proposed implementation

Release Notes

Adds litToBigInt[Option], litToBoolean[Option], and litToDouble[Option] APIs. Deprecates previous isLit, litArg, litValue APIs, since those expose implementation details (LitArg), do not have Option variants, and may not return the type desired (BigInt/Boolean/Double).
[internal] literals now specified in Bindings, instead of in Data subtypes.
[internal] adds support for Bundle literals by introducing a Bundle literal binding that contains a map of leaf subelements to their literal values.
~~[internal] ids are now persistent through runs, necessary to allow Bundle literals outside a Builder context (otherwise, a new IdGen is created for each element, and the ids alias).~~
[internal] HasId hashcode and equals reverted to the Java Object defaults, which allows Data created outside a Builder context to be Map-indexable
[internal] Created a ChiselContext (like DynamicContext) for global operations (like _id creation) that can be done outside a Builder context.
[internal] lref/ref checks are more strict, but Bindings should ensure they don't trip, and cases where they do trip are likely to be FIRRTL errors.
[internal] Made accesses to DynamicContext more strict, and will throw an error in most cases. The exceptions are error logging (errors are discarded) and getting the current Module (which will return None if outside a Builder context).

Other notes:

Support for bundle literal extractors is not yet implemented (the TODO is intentional), proposal is to have litToBigInt[Option] do the right thing. Note that bundle subelement literals can be extracted.
Support for generating Bundle literal constructors by macro annotation will be in a future PR. This only provides the internal framework to support Bundle literals at all. A mockup of what the generated code could look like is in the Bundle literal test case.
litArg, litValue, isLit are deprecated globally, even in Chisel context. If anyone really cares (as in, this causes problems for someone), I can move those into the compatibility layer without the deprecation warning through implicit conversions.

ducky64 · 2018-05-11T23:22:56Z

Changes:

HasId hashcode and equals reverted to the Java Object defaults, which allows Data created outside a Builder context to be Map-indexable. I can't seem to remove HasId.hashCode or HasId.equals completely without triggering a gnarly scalac error, so it just calls the superclass method. Funnnnnn...
Created a ChiselContext (like DynamicContext) for global operations (like _id creation) that can be done outside a Builder context.
Made accesses to DynamicContext more strict, and will throw an error in most cases. The exceptions are error logging (errors are discarded) and getting the current Module (which will return None if outside a Builder context).

chick · 2018-05-23T23:25:02Z

Added a test
"literals declared outside a builder context" should "compare with those inside builder context"
that shows errors with seemingly reasonable use cases.
Please investigate. Answer may that they are not reasonable use cases in which case better errors would be nice, but best would be if this test worked

ducky64 · 2018-05-24T18:59:31Z

It appears that we current don't support === on anything but UInt/etc. The assert appears to be routed to chisel3.assert.apply(cond: Boolean), which was a workaround / hack to allow Scala asserts to coexist with Chisel, and something (possibly ScalaTest) is turning the Bundle === into a Boolean.

I've fixed the chisel3.core.BundleLitBinding cannot be cast to chisel3.core.ElementLitBinding issues, Element.ref wasn't handling BundleLitBindings correctly. Partial Bundle literals may be broken.

ducky64 · 2018-05-24T18:59:57Z

Also, those tests probably don't belong in LiteralExtractorSpec, since they deal more with Bundle literals than literal extractors.

jackkoenig · 2018-05-24T19:17:41Z

The aliasing between Chisel === and ScalaTest === is annoying. I usually work around it by defining test modules that need === outside of the ScalaTest class. Maybe there's a better way though

ducky64 · 2018-05-24T19:17:59Z

It might also not be a bad idea to discuss renaming assert because of the pain it's causing with ScalaTest.

jackkoenig · 2018-06-01T18:03:07Z

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

+    case Some(litArg) => Some(litArg.num)
+    case _ => None
+  }
+


Cleaner as

override def litToBigIntOption: Option[BigInt] = litArgOption.map(_.num)

Fixed, thanks!

chick

This looks ok to me. There is some reasonable name changing like DynamicContext to ChiselContext that creates a lot of diff's but overall looks ok. This PR REQUIRES documentation in the wiki and probably some more tests after that doc is written. I'd suggest at least one more approval before merging

jackkoenig

A few comments, questions, suggestions

jackkoenig · 2018-06-01T18:03:21Z

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

+  private[chisel3] def litIsForcedWidth: Option[Boolean] = litArgOption match {
+    case Some(litArg) => Some(litArg.forcedWidth)
+    case _ => None
+  }


Similar to the above

jackkoenig · 2018-06-01T18:09:36Z

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

+    case Some(intVal) if intVal == 0 => Some(false)
+    case Some(intVal) => throwException(s"Boolean with unexpected literal value $intVal")
+    case None => None
+  }


I think better as something like:

def litToBooleanOption: Option[Boolean] = litToBigIntOption.map { intVal => if (intVal == 1) true else if (intVal == 0) false else throwException(s"Boolean with unexpected literal value $intVal") }

or

def litToBooleanOption: Option[Boolean] = litToBigIntOption.map { case intVal if intVal == 1 => true case intVal if intVal == 0 => false case intVal => throwException(s"Boolean with unexpected literal value $intVal") }

I like option 2 for the (grammatical) parallelism; fixed.

jackkoenig · 2018-06-01T18:10:36Z

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

+      val multiplier = math.pow(2, binaryPoint.get)
+      Some(intVal.toDouble / multiplier)
+    case _ => None
+  }


I guess across the board I think it's better to leave off the case _ => None by just using .map on Options

jackkoenig · 2018-06-01T18:28:49Z

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

  def isLit(): Boolean = litArg.isDefined

+  // If this is an element literal, returns the LitArg bound to it.
+  // INTERNAL API, but this isn't protected to allow bundle literal constructors.


I think this should be a ScalaDoc indicating that this is really an internal API. Does it need to be public or could it be protected if it's accessed by Bundle literal constructors which are methods on children of Bundle right?

Looking into more creative solutions to this.

The basic issue is this:

[error] Access to protected method elementLitArg not permitted because [error] prefix type chisel3.Bool does not conform to [error] class MyBundle where the access take place [error] clone.b -> bVal.elementLitArg.get

though MyBundle is a type of Bundle which is a type of Data, accessing the override elementLitArg defined in Element seems to cause problems.

Fixed, added a protected Aggregate.litArgOfBits for this purpose, which depends on private[core] Bits.litArgOption

jackkoenig · 2018-06-01T18:34:39Z

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

+    case _ => None
+  }
+  @chiselRuntimeDeprecated
+  @deprecated("litValue deprecated, use litToBigInt or litTo*", "chisel3.2")


I don't think this should be deprecated. litToBigInt does the same thing just with a different name so I see no reason to deprecate this for a slight name change. I agree with deprecating litArg since we generally don't want people accessing the Chisel IR though.

I think litValue is ambiguous (for example, on FixedPoint you would not get the original value out) and inconsistent with the new API style (litTo[Type]), and we should encourage one way of doing things, instead of having two equivalent representations (yes, I know Scala does this, and I think they're wrong).

We could also standardize on litValue instead of litToBigInt, though I think the latter is more clear. And the former is not consistent with the other literal extractors we would want (especially for the new testers), like litToBoolean or litToDouble.

jackkoenig · 2018-06-01T20:20:27Z

src/main/scala/chisel3/util/Conditional.scala

@@ -28,8 +28,8 @@ class SwitchContext[T <: Bits](cond: T, whenContext: Option[WhenContext], lits:
  def is(v: Iterable[T])(block: => Unit): SwitchContext[T] = {
    if (!v.isEmpty) {
      val newLits = v.map { w =>
-        require(w.isLit, "is conditions must be literals!")
-        val value = w.litValue
+        require(w.litToBigIntOption != None, "is condition must be literal")


w.litToBigIntOption != None => w.litToBigIntOption.isDefined

edwardcwang · 2018-06-06T18:34:47Z

Agree with Chick on documentation. From a user perspective that actually might be more interesting to review.

This shows errors comparing literals

ducky64 · 2018-06-29T22:12:15Z

Resolution from today's meeting: don't deprecate litValue, because modifying public APIs is bad. Instead, litToBigInt will be removed to avoid duplication of features.

Alternatives explored:

Keep both litValue and litToBigInt, marking one as "legacy" in documentation. I think that marking things as legacy in documentation is near-worthless, since it's far from guaranteed that people read the documentation. Especially if we do our jobs right and method names are descriptive.
Introduce another level of deprecation, below the current Scala deprecation level, and use this to mark litValue as a legacy function (so there will be no expectation that it will be removed in the future and existing code needs to be changed). Having some visible warning / informational message automatically generated reduces issues with people not reading documentation. I personally like this one, but there were concerns about additional complexity.
Have both APIs. I think this just unnecessarily adds complexity, now you have to learn two names for equivalent functionality, and there will be the inevitable question of "what do these do that are different" followed by "wtf, why".

The downside is that litValue will be named inconsistently from litToDouble or litToBoolean, which I think are useful for extracting FixedPoint literals or using Bools in Scala conditionals.

ducky64 · 2018-07-01T19:51:58Z

It seems like any literal naming stability issues would be caused by code in UserModule.scala:

    // All suggestions are in, force names to every node.
    for (id <- getIds) {
      id match {
        case id: BaseModule => id.forceName(default=id.desiredName, _namespace)
        case id => id.forceName(default="_T", _namespace)
      }
      id._onModuleClose

Having that filter by binding type probably names sense (in particular, ignore LitBinding, maybe also PortBinding since Ports need to be user-specified anyways) and should fix this.

jackkoenig · 2018-07-03T20:09:48Z

It's my fault for not providing a better test, but this currently does not preserve the non-lit naming behavior of #826. I'll provide a better test

jackkoenig · 2018-07-03T20:35:00Z

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

@@ -62,6 +62,12 @@ sealed abstract class Aggregate extends Data {
    }
  }

+  override def litOption = ???  // TODO implement me


Is this TODO intended to be resolved in this PR or later?

This is meant to be implemented later. Literal extractors on Bundles would be a new feature.

jackkoenig · 2018-07-03T20:52:32Z

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

-
-      requireIsHardware(this, "bits to be indexed")
-      pushOp(DefPrim(sourceInfo, Bool(), BitsExtractOp, this.ref, ILit(x), ILit(x)))
-    }


Just for the historical record:

These are minor constant propagations done on literals. I did some archaeology to understand why they were there in the first place. They date to very early chisel3 7ada20e and appear to be largely to work around bugs in order to get riscv-mini compiling. Most of them have already been removed but a couple seem to have stuck around. I agree they should be removed.

jackkoenig · 2018-07-03T20:55:56Z

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

+    case Some(ElementLitBinding(litArg)) => litArg
+    case Some(BundleLitBinding(litMap)) => litMap.get(this) match {
+      case Some(litArg) => litArg
+      case _ => throwException(s"internal error: DontCare should be caught before connect")


I get that this is an internal (and thus unexpected) error, but is it wise to specifically mention "connect" here?

Fair, changed it to say ref instead.

jackkoenig · 2018-07-03T21:01:13Z

It seems like any literal naming stability issues would be caused by code in UserModule.scala:
{code}
Having that filter by binding type probably names sense (in particular, ignore LitBinding, maybe also PortBinding since Ports need to be user-specified anyways) and should fix this.

Yes I agree. And in fact then we can do much more than #826 did because we could possibly prevent Bundle types from impacting temporary name suffixes as well! I'm okay with not doing that whole hog in this PR, just so long as we preserve the behavior from #826

…Module

into bundlelits

ducky64

UserModule forceName is now filtered by binding type. BlackBoxes will not forceName at all (it probably doesn't make sense to have temporaries on BlackBoxes anyways).

ducky64 · 2018-07-04T22:06:38Z

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

@@ -62,6 +62,12 @@ sealed abstract class Aggregate extends Data {
    }
  }

+  override def litOption = ???  // TODO implement me


This is meant to be implemented later. Literal extractors on Bundles would be a new feature.

ducky64 · 2018-07-04T22:13:28Z

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

+    case Some(ElementLitBinding(litArg)) => litArg
+    case Some(BundleLitBinding(litMap)) => litMap.get(this) match {
+      case Some(litArg) => litArg
+      case _ => throwException(s"internal error: DontCare should be caught before connect")


Fair, changed it to say ref instead.

jackkoenig

This looks great! Thank you for all of your hard work on this

ucbjrl · 2018-07-09T18:04:03Z

Unfortunately, commit 46f0aa3 / 93eedef breaks SimpleTBSpec tests in dsptools:

STARTING test_run_dir/SimpleTB.SimpleTBSpec1077962831/VSimpleLitModule
[info] [0.002] SEED 1531158343599
java.util.NoSuchElementException: None.get
	at scala.None$.get(Option.scala:347)
	at scala.None$.get(Option.scala:345)
	at chisel3.internal.HasId$class.getRef(Builder.scala:114)
	at chisel3.core.Data.getRef(Data.scala:201)
	at chisel3.internal.firrtl.Node.fullName(IR.scala:56)
	at chisel3.internal.firrtl.Slot.fullName(IR.scala:105)
	at chisel3.internal.HasId$class.instanceName(Builder.scala:119)
	at chisel3.core.Data.instanceName(Data.scala:201)
	at dsptools.DspTesterUtilities$.getName(DspTesterUtilities.scala:57)
	at dsptools.DspTester.expect(DspTester.scala:301)
	at dsptools.DspTester.expect(DspTester.scala:248)
	at dsptools.DspTester.expect(DspTester.scala:246)
	at SimpleTB.EasyPeekPoke$class.check(SimpleTBwGenTypeOption.scala:54)
	at SimpleTB.PassLitTester.check(SimpleTBwGenTypeOption.scala:259)
	at SimpleTB.EasyPeekPoke$class.checkP(SimpleTBwGenTypeOption.scala:34)
	at SimpleTB.PassLitTester.checkP(SimpleTBwGenTypeOption.scala:259)
	at SimpleTB.PassLitTester$$anonfun$8.apply(SimpleTBwGenTypeOption.scala:271)
	at SimpleTB.PassLitTester$$anonfun$8.apply(SimpleTBwGenTypeOption.scala:271)
	at scala.collection.Iterator$class.foreach(Iterator.scala:891)
	at scala.collection.AbstractIterator.foreach(Iterator.scala:1334)
	at scala.collection.IterableLike$class.foreach(IterableLike.scala:72)
	at scala.collection.AbstractIterable.foreach(Iterable.scala:54)
	at SimpleTB.PassLitTester.<init>(SimpleTBwGenTypeOption.scala:271)
	at SimpleTB.SimpleTBSpec$$anonfun$17$$anonfun$apply$6.apply(SimpleTBwGenTypeOption.scala:375)
	at SimpleTB.SimpleTBSpec$$anonfun$17$$anonfun$apply$6.apply(SimpleTBwGenTypeOption.scala:374)
	at chisel3.iotesters.Driver$$anonfun$execute$1$$anonfun$apply$mcZ$sp$1$$anonfun$apply$mcZ$sp$2.apply$mcZ$sp(Driver.scala:65)
	at chisel3.iotesters.Driver$$anonfun$execute$1$$anonfun$apply$mcZ$sp$1$$anonfun$apply$mcZ$sp$2.apply(Driver.scala:64)
	at chisel3.iotesters.Driver$$anonfun$execute$1$$anonfun$apply$mcZ$sp$1$$anonfun$apply$mcZ$sp$2.apply(Driver.scala:64)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58)
	at chisel3.iotesters.Driver$$anonfun$execute$1$$anonfun$apply$mcZ$sp$1.apply$mcZ$sp(Driver.scala:63)
	at chisel3.iotesters.Driver$$anonfun$execute$1$$anonfun$apply$mcZ$sp$1.apply(Driver.scala:39)
	at chisel3.iotesters.Driver$$anonfun$execute$1$$anonfun$apply$mcZ$sp$1.apply(Driver.scala:39)
	at logger.Logger$$anonfun$makeScope$1.apply(Logger.scala:129)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58)
	at logger.Logger$.makeScope(Logger.scala:127)
	at chisel3.iotesters.Driver$$anonfun$execute$1.apply$mcZ$sp(Driver.scala:39)
	at chisel3.iotesters.Driver$$anonfun$execute$1.apply(Driver.scala:39)
	at chisel3.iotesters.Driver$$anonfun$execute$1.apply(Driver.scala:39)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58)
	at chisel3.iotesters.Driver$.execute(Driver.scala:38)
	at dsptools.Driver$$anonfun$execute$1.apply$mcZ$sp(Driver.scala:29)
	at dsptools.Driver$$anonfun$execute$1.apply(Driver.scala:26)
	at dsptools.Driver$$anonfun$execute$1.apply(Driver.scala:26)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58)
	at dsptools.Driver$.execute(Driver.scala:26)
	at SimpleTB.SimpleTBSpec$$anonfun$17.apply(SimpleTBwGenTypeOption.scala:374)
	at SimpleTB.SimpleTBSpec$$anonfun$17.apply(SimpleTBwGenTypeOption.scala:368)
 ...

due to the fact that _ref is None.

jackkoenig · 2018-07-09T18:08:08Z

I also found that 95f1e17 breaks connecting the result of Mux on aggregates in compatibility mode, I have a fix for that coming.

ducky64 requested a review from jackkoenig May 11, 2018 19:45

chick self-requested a review May 18, 2018 20:28

ducky64 mentioned this pull request May 24, 2018

Aggregates can now be marked as literals. #694

Closed

jackkoenig mentioned this pull request Jun 1, 2018

Literals set the ref so that they no longer get named #826

Merged

jackkoenig reviewed Jun 1, 2018

View reviewed changes

chick approved these changes Jun 6, 2018

View reviewed changes

jackkoenig requested changes Jun 6, 2018

View reviewed changes

ducky64 and others added 15 commits June 28, 2018 12:34

Infrastructure for bundle literals

fbad34b

work on new style literal accessors

c23faf0

refactoring of lit and ref implementations

c2a51df

bundle literal mockup, but broken =(

ab080ea

lol=(

20634c9

Run-unique ids

75d0ccf

style

927e604

delete debugging stuff

4948abe

broken

fb53b1e

still broken

aa927ba

unbroken

761fad6

Add new test LitInsideOutsideTester

f109803

This shows errors comparing literals

Comment out assertion test, fix ref generation

d655175

Add BundleLiteralSpec

f888adf

properly fix undefined clock/reset issues

7fa963d

ducky64 force-pushed the bundlelits branch from 3e98177 to 7fa963d Compare June 28, 2018 21:53

Change [public] Data.elementLitArg => [protected] Aggregate.litArgOfBits

418d7cc

Prefer litValue, eliminate litToBigInt

9a3cd9f

Add test that UInt, SInt, and FP literals do not impact naming

0292722

jackkoenig reviewed Jul 3, 2018

View reviewed changes

ducky64 added 4 commits July 3, 2018 16:01

Remove forceName rom BlackBox/ExtModule, filter out forceName in User…

93eedef

…Module

Merge branch 'bundlelits' of https://github.com/freechipsproject/chisel3

ac34d81

into bundlelits

Fix strict namer

62603b0

Change wording of internal failure

97907ca

ducky64 force-pushed the bundlelits branch from a7087c9 to 97907ca Compare July 4, 2018 22:15

ducky64 commented Jul 4, 2018

View reviewed changes

jackkoenig approved these changes Jul 4, 2018

View reviewed changes

Merge branch 'master' into bundlelits

4c652c7

ducky64 merged commit 5c32cd8 into master Jul 4, 2018

ducky64 deleted the bundlelits branch July 4, 2018 23:39

edwardcwang mentioned this pull request Jul 8, 2018

Vec literals #849

Closed

This was referenced Jul 9, 2018

Fix use of read-only refs on rhs of connect in compatibility mode #854

Merged

Ensure names work for bundles and literals. #853

Merged

Revert removal of bit extraction const prop for literals #857

Merged

jackkoenig added this to the 3.2.0 milestone Jul 13, 2018

shunshou mentioned this pull request Oct 31, 2018

DspComplex[T].zero not initialized ucb-bar/dsptools#143

Open

jackkoenig mentioned this pull request Nov 29, 2018

Fix width inferencing issue #952

Merged

ducky64 mentioned this pull request May 4, 2021

peek.litValue is confusing ucb-bar/chiseltest#298

Closed

Bundle Literals Framework #820

Bundle Literals Framework #820

Conversation

ducky64 commented May 11, 2018 • edited Loading

ducky64 commented May 11, 2018

chick commented May 23, 2018

ducky64 commented May 24, 2018

ducky64 commented May 24, 2018

jackkoenig commented May 24, 2018

ducky64 commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chick left a comment • edited Loading

Choose a reason for hiding this comment

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

edwardcwang commented Jun 6, 2018

ducky64 commented Jun 29, 2018

ducky64 commented Jul 1, 2018

jackkoenig commented Jul 3, 2018

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 Jul 3, 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

jackkoenig left a comment

Choose a reason for hiding this comment

ucbjrl commented Jul 9, 2018

jackkoenig commented Jul 9, 2018

ducky64 commented May 11, 2018 •

edited

Loading

chick left a comment •

edited

Loading