Aggregate Literals #921

grebe · 2018-11-01T09:05:44Z

This (in addition to a FIRRTL PR) is another stab at adding genuine bundle literals. It also broadens the names of things slightly- I think the bindings should probably be on aggregates rather than bundles so Records and Vecs can get in on the party.

The tests are dying in FIRRTL right now, but it seems to be generating FIRRTL that should work. The firrtl it generates should be cleaner, e.g.

{ a : { b : UInt<4>("h0") } }.a.b

should just be UInt<4>("h0").

The API is a bit clunky and could definitely be polished more, but I wanted to get feedback on this and firrtl stuff before going to far down that road.

Related issue: This FIRRTL PR and #805

Type of change: feature request

Impact: API modification

Development Phase: implementation

ducky64

I think conceptually this makes sense, though the implementation could use a bit of cleaning.

chiselFrontend/src/main/scala/chisel3/internal/firrtl/IR.scala

ducky64 · 2018-11-01T21:49:44Z

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

@@ -410,8 +410,7 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc {
  @chiselRuntimeDeprecated
  @deprecated("litArg is deprecated, use litOption or litTo*Option", "chisel3.2")
  def litArg(): Option[LitArg] = topBindingOpt match {
-    case Some(ElementLitBinding(litArg)) => Some(litArg)
-    case Some(BundleLitBinding(litMap)) => None  // this API does not support Bundle literals


This is intentionally there to note that this API does not and should not support Bundle/Aggregate literals. Code-wise does nothing, it's more documentation.

I'm not sure what you mean. Should litArg not return an AggregateLit? Regardless, I think it's OK to move the functionality to the LitBinding unapply, unless you want it explicitly documented here.

It's a legacy function from before the Bundle/Aggregate literal days, so code calling this probably wasn't designed with Bundle literals in mind and some of the more edge cases like partial Bundle literals. So this just takes the sledgehammer approach of doesn't work with new features.

Perhaps similar functionality with a new name should be added. Writing a function that returns a bundle literal is a lot cleaner when you can call .litArg- look at a9bc2c8 to see how things get uglier when you can't use litArg.

ducky64 · 2018-11-01T21:50:47Z

chiselFrontend/src/main/scala/chisel3/internal/firrtl/IR.scala

-  if (forcedWidth) {
-    require(widthArg.get >= minWidth,
-      s"The literal value ${num} was elaborated with a specified width of ${widthArg.get} bits, but at least ${minWidth} bits are required.")
+class AggregateLit(val litMap: Seq[(String, Data, LitArg)]) extends LitArg {


Why Seq instead of Map?

Can you unify the redundant information (you have both the name and a Data, can you just have one) instead of overencoding data and having potential for consistency issues?

I have a Seq because I would like the order to match the bundle- I think this is important for things like asUInt to work as expected, but perhaps that is incorrect. I think asUInt will take a literal like { a : UInt("h3"), b : UInt("h4") } and use a asUInt primop rather than emitting the UInt directly, which would require the order to be correct.

Re: the redundancy, I agree it's not great. I'm not really sure what the right thing to do is, though. I need names to correctly emit firrtl, but the internal chisel stuff wants the Data. I think it's easier to go from names to data via the bundle API, but I wanted to get your thoughts on that.

I find names somewhat annoying because it seems to be going in a stringly-typed direction.

I think I also thought about names vs. Data ref, and my implementation uses the Data ref. I think when you're emitting, you should reference the top-level Aggregate and get names from there.

I don't think having the Seq for ordering is a good idea, the ordering information then becomes stored in two places (in the Seq ordering and in the Bundle elements ordering) and non-DRY. I think it makes sense to use the Bundle elements as the authoritative data source, with this just providing additional data.

I agree with pawning as much work to FIRRTL as possible - doing things like const prop in the frontend makes it more likely to have subtle differences in behavior. And you have to keep your implementation in sync in two places.

My current approach is that when aggregates are bound as literals, the children get bound as literals too. I think this is a lot more straightforward than references to bundle elements look at topBindingOpt and then searching through a map. Data equality checking and hashCodes don't really work, right?

build.sbt

ducky64 · 2018-11-01T21:56:00Z

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

 // Literal binding attached to the root of a Bundle, containing literal values of its children.
-case class BundleLitBinding(litMap: Map[Data, LitArg]) extends LitBinding
+case class AggregateLitBinding(litArg: AggregateLit) extends LitBinding
+object AggregateLitBinding {


Why have this companion object? Should it be different than ElementLitBinding, which doesn't have this convenience API?

I can write it out every time, but there's only one flavor of AggregateLit. ElementLitBindings have multiple flavors.

I think whatever you choose it should be stylistically consistent with how everything else is done, which currently seems to be no companion object, building the IR *Lit node, and passing that to bindLitArg. Or you can refactor everything if you think that your style is significantly better.

src/test/scala/chiselTests/BundleLiteralSpec.scala

project/build.properties

grebe · 2018-11-01T23:20:16Z

Thanks for the feedback, @ducky64!

The tests were using litArg, which shouldn't be done.

grebe · 2018-12-27T21:59:16Z

This version binds children with literal bindings rather than child bindings. I think it's reasonable that you'd want to treat an element of a bundle literal as a literal, plus I think it's a simpler and more direct way to implement the functionality.

I went back to litArg not returning a bundle lit arg. This means it is a pain to put bundle lits inside bundle lits. Each bundle defines both Lit and LitArg.

Right now, it isn't too hard to write something that will die in firrtl. More checking should be done- names should be validated, aggregates should check when binding that the litargs have the right type for their fields, etc.

The top level API for creating a VecLit doesn't exist. I think the ideal way to implement this is to have an implementation of litArg for bundles. I'd like
VecLit(3.U, 4.U, 2.U)
and
VecLit(MyBundle.Lit(3.U, 4.U), MyBundle.Lit(2.U, 1.U))
to both work.

ducky64 · 2018-12-28T00:16:24Z

I wrote up my thoughts on bundle literal bindings in #805, but the argument for the current system is basically:

Having just LitBinding discards the parent pointer provided by ChildBinding (though you could have a ChildLitBinding or something, and an equivalent for DontCare). I don't think anything uses that parent pointer right now, though it could be useful in the future.
It's structurally different than ChildBinding - now there are two ways to bind Aggregate children: one immediate (in this literals case) and one indirect (pointer to parent, in every other binding). If parent pointer data isn't necessary, a different solution here may just be to get rid of ChildBinding completely and bind all Aggregate elements locally. Implications unclear. But I'd like the systems to be unified so there's less chance of gotchas from not realizing that there's two different ways something needs to be handled.
Rebinding is disallowed. Not currently a problem: we don't currently initialize ChildBinding on non-top-level-bound objects (it's set at binding time - though early initialization of ChildBinding would fail early when attempting to bind subfields of Aggregates), but were we to do that under current bind-once rules it wouldn't be possible to stack a LitBinding on top of that,

edwardcwang · 2019-03-15T20:39:38Z

Depends on FIRRTL PR

jackkoenig · 2023-03-19T01:57:07Z

Stale with too many conflicts, still a good idea though that we really ought to do.

grebe requested a review from a team as a code owner November 1, 2018 09:05

grebe mentioned this pull request Nov 1, 2018

Instantiating constants of type DspComplex[DspReal] ucb-bar/dsptools#144

Open

ducky64 requested changes Nov 1, 2018

View reviewed changes

grebe added 5 commits December 27, 2018 14:39

More bundle literals work

32f23e1

WIP: Bundle Literals

e95e9ba

Some refactoring for recent firrtl work.

755dfbc

Fix Driver compilation error

d68c56c

Update aggregate literals to bind their children with literal bindings.

68dd52c

grebe force-pushed the bundleLiterals2 branch from 75ec2ff to 68dd52c Compare December 27, 2018 20:38

grebe added 5 commits December 27, 2018 15:42

Remove dead code

e7275c2

Don't return a lit arg for bundles or vecs

3b3039e

Add some comments

f987072

OneHotMuxSpec was throwing a different error

4890cdf

litArg is deprecated and shouldn't work with bundles

a9bc2c8

The tests were using litArg, which shouldn't be done.

ducky64 added this to the 3.2.0 milestone Jan 18, 2019

ducky64 assigned grebe Jan 18, 2019

azidar modified the milestones: 3.2.0, 3.2.1 Mar 1, 2019

edwardcwang added the DO NOT MERGE label Mar 15, 2019

jackkoenig changed the title ~~Bundle literals2~~ Aggregate Literals Mar 2, 2020

albert-magyar modified the milestones: 3.2.1, 3.4.0 May 13, 2020

ducky64 mentioned this pull request Aug 7, 2020

Bundle asUInt ignores structural width #1510

Open

azidar modified the milestones: 3.4.0, 3.5.0 Oct 27, 2020

jackkoenig closed this Mar 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggregate Literals #921

Aggregate Literals #921

grebe commented Nov 1, 2018

ducky64 left a comment

ducky64 Nov 1, 2018

grebe Nov 1, 2018

ducky64 Nov 2, 2018

grebe Dec 27, 2018

ducky64 Nov 1, 2018

ducky64 Nov 1, 2018

grebe Nov 1, 2018 •

edited

Loading

ducky64 Nov 2, 2018

grebe Dec 27, 2018

ducky64 Nov 1, 2018

grebe Nov 1, 2018

ducky64 Nov 2, 2018

grebe commented Nov 1, 2018

grebe commented Dec 27, 2018

ducky64 commented Dec 28, 2018

edwardcwang commented Mar 15, 2019

jackkoenig commented Mar 19, 2023

Aggregate Literals #921

Aggregate Literals #921

Conversation

grebe commented Nov 1, 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

grebe Nov 1, 2018 • edited Loading

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

grebe commented Nov 1, 2018

grebe commented Dec 27, 2018

ducky64 commented Dec 28, 2018

edwardcwang commented Mar 15, 2019

jackkoenig commented Mar 19, 2023

grebe Nov 1, 2018 •

edited

Loading