Aggregates can now be marked as literals. #694

grebe · 2017-09-16T06:18:24Z

This is inspired by dsptools DspReal(), which is implemented as a Bundle with a UInt inside. Bundles currently can't be literals, which causes problems because it is totally natural to want a literal DspReal().

This is an initial stab at making literal bundles work in this context. It doesn't go anywhere towards addressing #417, although I think it does address #418 to an extent. This is also related to ucb-bar/dsptools#55 and perhaps ucb-bar/dsptools#87.

The big difference between how lits need to work for Aggregates and UInt/SInt/etc. is that UInt/SInt/etc aren't meant to be subclassed, whereas Aggregates are (especially Bundle and Record). Some API needs to be provided for users to mark things as literals (checking that they actually are).

Currently, I check by calling isLit() on all children. This is fine for UInt/SInt/etc., but is problematic for Aggregates because they currently don't mark themselves as lits. I added a private var that gets set to something when you try to mark an Aggregate as a lit, but the actual value is ignored because I don't do anything with it. I'm not a fan of doing this, but not familiar enough with the backend to know what a better alternative would be.

@ducky64 @jackkoenig @chick

grebe · 2017-09-16T06:22:30Z

src/test/scala/chiselTests/BundleLiterals.scala

+
+class BundleWithLit(lit: Option[Int]=None) extends Bundle {
+  val internal = lit.map(_.U).getOrElse(UInt(4.W))
+  override def cloneType = new BundleWithLit(lit).asInstanceOf[this.type]


Is this a mistake? I just realized that cloneType of a lit should not be a lit. This should probably be new BundleWithLit().asInstanceOf[this.type]

I think that's right, that cloneType should not return a Lit.

edwardcwang

Bundle literals is something that's been brought up many times at the Chisel meetings. This might work for just getting lits to be emitted properly for the dsptools usecases you linked, though I think more thought is needed on how bundle literals should be generally. @ducky64

edwardcwang · 2017-09-16T06:28:42Z

src/test/scala/chiselTests/BundleLiterals.scala

+  behavior of "Bundle literals"
+
+  it should "build the module without crashing" in {
+    println(chisel3.Driver.emitVerilog( new BundleWithLitModule ))


You may want to compare firrtl output, instead of just emitting Verilog.

Fair enough, I just did a "it's doing something" test to start with

edwardcwang · 2017-09-16T06:29:17Z

src/test/scala/chiselTests/BundleLiterals.scala

+object BundleWithBundleWithLit {
+  def apply(x: Int) = {
+    val ret = new BundleWithBundleWithLit(Some(x))
+    Aggregate.LitBind(ret)


Seems strange to have to call this in user code; why is it here?

This is patterned after UIntFactory, which isn't user code b/c you don't subtype UInt, but users do subtype bundles. I suppose you could have an aggregate try to detect if its members are literals, but that seemed to be a bigger change so I didn't take that step.

edwardcwang · 2017-09-16T06:31:09Z

src/test/scala/chiselTests/BundleLiterals.scala

+  val io = IO(new Bundle {
+    val out = Output(BundleWithLit())
+  })
+  io.out := BundleWithLit(3)


Is this the intended bundle literal syntax? Also, what about bundles with multiple members?

I don't know if I'd say the literal syntax, but it is the kind of syntax we use with DspReal().
You're companion object could take multiple parameters

ducky64 · 2017-09-16T07:12:00Z

Agree with @edwardcwang, the frontend definitely needs more thought - from the testcases, the user has to do a lot of work to write bundle literal constructors. As for this just being a backend implementation, I need to cache back in the bindings and bundles system, but as far as I can remember, is there a reason a literal Bundle needs to be bound as such?

Literal bundle constructors (possibly implemented using macros) is on the docket of stuff for 3.1.0, and will likely be a part of Testers2. As a timeframe estimate, both are probably at least two months out, there's a list of more pressing stuff in terms of Chisel3 engineering dev.

grebe · 2017-09-16T07:28:08Z

I'm trying to remember the exact issues I was seeing in dsptools, but our solution is to create a wire and assign to it with literals. Sometimes you don't want to create the wire- I can't remember what situations cause problems (maybe if you're passing around a literal bundle just to get a width out of it or call cloneType in an IO? Like new TypeParameterizedModule(proto=DspReal(0.3))). This means that it is somewhat confusing whether something will create a wire or not.

It's more than just the annoyance of creating new wires, there's a reason UInts/SInts/etc. treat literals specially.

I agree that the example of the frontend is clunky. I think it can be made easier, e.g. a def MarkAsLit[T <: Aggregate](in: T): T so your companion object would just have def apply(x: Int) = MarkAsLit(new Whatever(x)) (doesn't eliminate the boilerplate or fragility, but makes it less ugly).

Do you see literal bundle constructors as changing how lits work at the core? It seems to me that whatever is implemented will have to do something to set litArg to Some(...). Perhaps it will do so by rewriting literal bundles to be of a subtype of Bundle that sets litArg, rather than the hackish private var trick.

ducky64 · 2017-09-16T07:36:17Z

I'm not sure what the implementation for bundle literals should look like yet, that's something that needs more thought, possibly a fresh look into refactoring the entire literals system if warranted. That would be something that would be RFC-level, where it's probably worth having a detailed proposal and analysis before we start implementing away.

chick · 2017-09-17T17:32:06Z

src/test/scala/chiselTests/BundleLiterals.scala

+import org.scalatest._
+
+class BundleWithLit(lit: Option[Int]=None) extends Bundle {
+  val internal = lit.map(_.U).getOrElse(UInt(4.W))


What is the OrElse doing?

Providing a default width

grebe · 2017-09-17T18:50:25Z

@ducky64 I agree that if you are going to tackle the entire bundle literals problem (with implications in chisel, testers, and firrtl) that's a large task that should be done thoughtfully and with a process. I should be clear that what I'm pushing for here is to use the existing bindings infrastructure to be able to bind bundles as literals and nothing more ambitious than that.

I just pushed a cleaner version of the original idea. The examples are pretty clean, I think. It adds another nuance to how cloneType can fail (if it returns something that is bound), but this is already something that can happen and cause problems if you aren't careful.

One slightly nuanced thing is that empty Bundles should not be marked as literal. They're used all over the place in our testers, which will result in a module complaining that its IO is hardware when it should be chisel.

For whatever reason a Vec test is failing, I haven't tracked down why. I don't think it's a big deal, it's probably an easy fix if you understand how Vecs work.

grebe · 2017-09-17T18:51:55Z

src/main/scala/chisel3/testers/BasicTester.scala

@@ -13,7 +13,7 @@ import internal.sourceinfo.SourceInfo

 class BasicTester extends Module() {
  // The testbench has no IOs, rather it should communicate using printf, assert, and stop.
-  val io = IO(new Bundle())
+  val io = IO(new Bundle {val dummy = Input(UInt(0.W))})


Oops, forgot to clean this up, this is no longer necessary.

ducky64

Some code issues, but more importantly is the larger structural discussion.

Bindings questions

Currently, things are bound at the top, and leaf elements only have a ChildBinding. This proposal messes with that in the case of literals, now literals at the leaves (and all the way up the hierarchy) would have LitBindings. We can certainly rework how the binding system works, but let's try to keep things consistent.

Potential alternatives include having the top-level store the literal data of the children (but this decouples literal data from the element node) or having the literal data be independent of the binding (leaf elements continue to have ChildBinding, but also have their literal values locally - though this brings up another avenue for data inconsistency / disagreement).

Additional questions arise on the composability of literal bundles.

Frontend questions

The way the examples are structured doesn't seem like how I usually see Chisel code (pass in bitwidths instead of Data prototypes, which can either be bound or not). In fact, one of the goals of the cloneType and friends refactor was to make people strictly separate Chisel types vs. hardware nodes, which this style breaks.

Composability concerns should also be part of the frontend, though it would appear composition of literals is not an issue in your implementation.

Additionally, providing solutions for backwards compatibility would be nice. For example, we really shouldn't modify the Decoupled (for example) semantics because it's in wide use, but there should be a way to add Decoupled literals.

ducky64 · 2017-10-23T21:31:51Z

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

+  }
+  override def litArg = {
+    // TODO[grebe] make a lit for Aggregates
+    childLitArgs.map { x => ULit(0, 1.W) }


Things should fail noisily if this is an unexpected case instead of propagating bad values

ducky64 · 2017-10-23T21:38:13Z

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

+  }
+
+  protected[core] override def hasBinding = {
+    checkBinding


This looks kind of hacky, kind of like a game of whackamole to insert checks into all the right places, it would make more sense for higher-level code to apply the binding as it is being set to a literal.

The problem is that bind() is private[chisel3] which makes it impossible to do that in user-land right now.

ducky64 · 2017-10-23T21:39:37Z

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

@@ -570,6 +608,10 @@ class Bundle(implicit compileOptions: CompileOptions) extends Record {
        Builder.exception(s"Parameterized Bundle ${this.getClass} needs cloneType method")
        this
    }
+
+    require(!newBundle.isLit, s"Bundle ${this.getClass} has a bad cloneType: it should never return a literal")


This require won't fire if the user overrides cloneType (essentially always given your current examples), right?

ducky64 · 2018-05-24T20:53:43Z

Superseded by #820 (with more discussion in #805), does that get closer to addressing your use cases?

grebe · 2018-05-24T21:07:40Z

Yeah, this should be closed.

Aggregates can now be marked as literals.

f5991f1

grebe commented Sep 16, 2017

View reviewed changes

edwardcwang requested changes Sep 16, 2017

View reviewed changes

edwardcwang requested a review from ducky64 September 16, 2017 06:33

chick reviewed Sep 17, 2017

View reviewed changes

Slightly different approach for the same idea

54f857b

grebe commented Sep 17, 2017

View reviewed changes

Fix indent error

e0a8207

ducky64 reviewed Oct 23, 2017

View reviewed changes

Add a user-level testcase

cb45e83

ducky64 mentioned this pull request Apr 13, 2018

[RFC] Bundle Literals #805

Closed

ducky64 closed this May 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregates can now be marked as literals. #694

Aggregates can now be marked as literals. #694

grebe commented Sep 16, 2017 •

edited

Loading

grebe Sep 16, 2017

chick Sep 17, 2017

edwardcwang left a comment

edwardcwang Sep 16, 2017

grebe Sep 16, 2017

edwardcwang Sep 16, 2017

grebe Sep 16, 2017

edwardcwang Sep 16, 2017

grebe Sep 16, 2017

ducky64 commented Sep 16, 2017

grebe commented Sep 16, 2017

ducky64 commented Sep 16, 2017 •

edited

Loading

chick Sep 17, 2017

grebe Sep 17, 2017

grebe commented Sep 17, 2017 •

edited

Loading

grebe Sep 17, 2017

ducky64 left a comment •

edited

Loading

ducky64 Oct 23, 2017

ducky64 Oct 23, 2017

edwardcwang Oct 23, 2017

ducky64 Oct 23, 2017

ducky64 commented May 24, 2018

grebe commented May 24, 2018

Aggregates can now be marked as literals. #694

Aggregates can now be marked as literals. #694

Conversation

grebe commented Sep 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardcwang 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

ducky64 commented Sep 16, 2017

grebe commented Sep 16, 2017

ducky64 commented Sep 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grebe commented Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

ducky64 left a comment • edited Loading

Choose a reason for hiding this comment

Bindings questions

Frontend questions

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 May 24, 2018

grebe commented May 24, 2018

grebe commented Sep 16, 2017 •

edited

Loading

ducky64 commented Sep 16, 2017 •

edited

Loading

grebe commented Sep 17, 2017 •

edited

Loading

ducky64 left a comment •

edited

Loading