search

chipsalliance / chisel

Chisel: A Modern Hardware Design Language
https://www.chisel-lang.org/
Apache License 2.0
4.01k stars 602 forks source link

[RFC] Bundle Literals #805

Closed ducky64 closed 5 years ago

ducky64 commented 6 years ago

(supersedes #418 with a more detailed proposal of syntax and thoughts on implementation concerns, supersedes #694 with an alternative, I think cleaner, implementation path)

Motivation

I don't think much needs to be said here. Main use cases would be in testing (for concise and simple testvector specification) and in initializing Bundles with RegInit and WireInit. Maybe also ROMs, but there's a whole other issue for that.

Note: it's possible to construct Bundle-literal-like objects by casting from a UInt with asTypeOf, but specifying anything but all zeros requires knowledge of how Bundles are bit-packed, which is not great. This also requires a Builder context, making it incompatible with testers.

Proposed syntax

Given a concrete instance of a Bundle type, an autogenerated (by macro annotation) Lit function would return a new literal of that type.

For example, if I have

@bundle
class MyBundle(val width: Int) extends Bundle {
  val a = Bool()
  val b = UInt(width.W)
}

I would be able to do:

myBundleIo := (new MyBundle(8)).Lit(a=true.B, b=255.U)

Edge cases

It is not necessary to be in a Chisel Builder context to construct a Bundle literal (barring some logic construction being in the Bundle definition, which we have no control over). This allows its use in testers.

Using keyword arguments allows the partial specification of literals. Current proposal is for everything not specified to be treated as DontCare.

Bundle literals will take a literal interpretation of DontCare, that is, DontCare means DontCare (and might show up as X-like in simulation) instead of DoesntExist (where the previous value on the wire, if any, would not be overwritten).

Open questions

Should keyword arguments be used? On the plus side, it allows a partial specification, however, it is incompatible with def macros, which are used for source locators. Note that currently, literals do not provide source locators, but this could prevent compatibility with def macros in the future. (current proposal will be to use keyword arguments)

Because Bundles can take parameters, the current proposal requires a literal to be constructed given a Bundle instance (so Lit would be a method on an instance, instead of on a companion object). Does it make sense to have a shorthand version in the companion object when no parameters are required? For example, if I defined:

@bundle
class MyBundle extends Bundle {
  val a = Bool()
  val b = UInt(8.W)
}

should MyBundle.Lit(a=true.B, b=255.U) be allowed?

Implementation concerns

Dealing with LitArg

Literal values are currently stored in the Data subtype objects with an immutable litArg. However, this requires literals to be known at instantiation time, which is not possible because fields are instantiated in the Bundle itself, before the literal constructor is run.

Proposal is to move the litArg data into LitBinding, which probably makes more sense anyways. Note that this would result in a loss of (internal, within Chisel, not user-visible) type safety, since LitBinding would need to take values for UInt, Bool, and FixedPoint.

Bindings

Currently, hardware is bound either as a ChildBinding (element of a larger Aggregate) or some kind of leaf binding like LitBinding (indicating an Element is a literal). There are two proposals for binding Bundle literals:

Bindings stay top-level

In this approach, LitBinding could contain a map of subelements to their arguments. This works very well with the current structure implementation-wise, however, requires indirection to look up the value of a literal given its Data subtype object. Getting a sub-Bundle literal may also be costly.

It may be possible to have an inconsistent map: containing missing literal values, or extra literal values. Missing literal values could be interpreted as DontCare, or cause an error.

Allow intermediate hierarchy nodes to have top-level bindings

The possibilities here are to either have a ChildLitBinding (which has a parent pointer and a literal value) or to drop the parent pointer (ChildBinding) entirely and only have a LitBinding on leaves.

The main issue with the current structure is that Data objects can't be rebound. It might make sense to allow additional bindings: for example, attempting to rebind a Data that has an existing ChildBinding with a new LitBinding will replace the ChildBinding with a ChildLitBinding. Obviously, it's also possible to allow rewriting Bindings, but isn't an elegant solution.

Structural inconsistency is a possibility here, some leaves may be missing their additional LitBinding. In those cases, the system can either raise an error (strict mode, guarding against implementation errors), or treat it as DontCare. But unlike the 'Bindings stay top-level' proposal, it is impossible to have extra literal values.

Fixing n^2 Bundle instantiation

A somewhat related issue is that Bundle instantiation could be n^2 in clones. That is, each Bundle clone must clone its fields, done recursively. Because of the immutable Data objects abstraction we want to present in Chisel and the possibility of aliasing, even in Bundles (a great example would be the use of genType in multiple fields, perhaps through an Input(...) or Output(...) binding) I think we want to retain cloning behavior, and I'm not sure if there's a way to avoid the n^2 clones.

A larger refactor that changes the behavior of Bundle-specific Input(...), Output(...) or (the proposed) Field(...) in an intelligent manner might address this problem, though that's future work. When this becomes a serious bottleneck.

Thoughts, ideas, and suggestions on anything in this proposal are appreciated.

Issue template

Type of issue: other enhancement

Impact: API addition (no impact on existing code)

Development Phase: proposal

aswaterman commented 6 years ago

It would be nice to be able to instantiate a bundle literal by initializing only some fields and assigning the others to DontCare, without having to list them all by name. Something like

(new MyBundle(8)).Lit(a=true.B, others=DontCare)

That obviously won't work, since the bundle could have a field called others. So maybe (new MyBundle(8)).LitDontCare(a=true.B) but with a less-bad name than LitDontCare.

grebe commented 6 years ago

For me, the biggest motivation is getting parity between user-defined types defined via bundle (e.g. DspComplex) to have parity with base chisel types like UInt, etc. It's very annoying to have type-generic circuits have different behavior.

I very much like keyword arguments. Is there a deep reason that def macros don't support them, or is that a hole the scala devs will eventually patch?

Does it make sense to move the litArg to the binding? On the one hand, they belong together in the sense that each exists iff the other exists. On the other hand, I don't think any other binding has metadata, right?

Re: @aswaterman's comment, perhaps there should be a hook in the bundle declaration to decide how unspecified arguments are treated. Maybe a val you can override that returns a map from element names to default values. The default should probably Don'tCare, but maybe there could be some bundles you extends like DefaultZeroBundle or something like that.

This is exciting!

ducky64 commented 6 years ago

@aswaterman The original proposal would have default everything not specified (assuming we use keyword args) to DontCare. I guess that's also an interesting point, should we require an explicit others (or similar) keyword argument or a different method (like LitDontCare) so the default one errors if underspecified?

Of those options, I would like a keyword argument like others, though the form (and how we can prevent name aliasing) that takes is up for debate. I'd like to avoid having two constructors (like Lit and LitDontCare) since i think it will increase the learning curve by the number of constructs available.

@grebe def macros not supporting keyword arguments seems to be an implementation limitation. There were apparently attempts to implement it, but none were clean. See https://stackoverflow.com/questions/34144734/how-can-i-get-scala-named-and-default-arguments-to-work-with-macros

As for bindings, they track metadata like the hardware type a node is associated with (like WireBinding, RegBinding, ...) and the enclosing Module it belongs to. I think a literal is a similar form, and it makes sense to track the value in there. Note that none of the existing bindings have values because they're all dynamic.

mwachs5 commented 6 years ago

I reiterate my earlier comment... would I have access to this value to turn it into a Scala Int? MyBundle(8).LitVal(a = true.B, b = 255.U).litVal -> 0x1FF ?

Again to fix problems like this: https://github.com/freechipsproject/rocket-chip/blob/master/src/main/scala/devices/debug/Debug.scala#L734

ducky64 commented 6 years ago

@mwachs5 I think getting a literal Bundle back as its bits numeric representation could be made to work. Might not be in the first version, but the foundation for it is there

Using the syntax above and testers2 literal extractors, it would probably look like (new MyBundle(8)).Lit(a = true.B, b = 255.U).litToBigInt => BigInt(0x1ff)

ducky64 commented 6 years ago

I've started looking at code and implementation details, so here's a high-level plan if anyone wants to critique.

Phase 1, infrastructure

The first PR, hopefully to be up later this week, will provide internal APIs for bundle literal construction.

Refactor literal bindings to contain literal values

For literal bindings, I'd going to use option 1 above, bindings stay top-level. Though this incurs a lookup cost for any subelement, I think that having one central definition of a node type is an important and convenient property.

LiteralBinding would also be subclassed into the specific type of data held, for example: Int, Boolean, and Bundle. BundleLiteralBinding would contain a map of bundle elements to their LiteralBindings. All bindings would check their literal binding for type correctness on access.

Redefine lref to look at binding

I'm not quite sure what lref means, but it seems to generate the FIRRTL node for a Chisel object. Right now, it doesn't check binding status, but it probably really should because it doesn't seem to make sense for a non-hardware type. The motivation here is to stricten up things to ensure literal bindings changes doesn't cause any inconsistency elsewhere.

Bundle literals and firrtl

FIRRTL support for literal bindings doesn't seem to be strictly necessary, especially since chisel3 breaks bulk connect operations into individual connects. However, this could make emitted FIRRTL containing bundle literals difficult to read, because they will not appear as a coherent object.

So emitting FIRRTL bulk connects and supporting bundle literals in FIRRTL, for the purposes of readability, would now be related issues

Phase 2, macros

The follow-on PR will provide a macro annotation to automatically generate and add the bundle literal construction code to any Bundle. This annotation will be added to all of the stock chisel3.util bundles.

The idea will be to define a @bundle annotation, which can be added to a class extending Bundle. It will add a .Lit (or .lit, depending on what everyone wants to see) method to a Bundle subclass instance that will create a literal given a unbound instance. All fields will be keyword arguments, and unfilled fields will default to DontCare.

There may be interplay with some of the less commonly used Bundle features, like Option elements. How those edge cases can be dealt with is currently uncertain, though runtime errors and checks may be necessary.

ducky64 commented 6 years ago

One issue that comes up is literal extractors.

Currently, we have Data.litArg, Data.litValue, and Data.isLit. Defining those with Bundle literals should be straightforward (lookup to root of tree), though it's unclear what should happen if it's explicitly undefined (equivalent to DontCare).

So we essentially have these scenarios:

  1. Literal with specified value
  2. Literal with DontCare (currently only possible as a partial bundle literal)
  3. Data that has a non-literal binding, or is not bound 1 and 3 are currently defined, 2 is more tricky.

Proposal

litArg, litValue and isLit will be deprecated. In the ambiguous case, or when applied to a Bundle literal, they will cause an elaboration time error - the method signatures are not expressive enough to return a correct answer, and the invoking code probably wasn't written with any of these edge cases in mind.

Introduce testers2-style literal extractors: Bits/Bool/FixedPoint.litToBigInt, Bool.litToBoolean, and FixedPoint.litToDouble (which will assert out on a non-literal input), as well as option variants that will gracefully return None. Aggregates will also have a litToBigInt defined to extract the packed-bits representation. This will runtime error out if the Aggregate contains a non-Bits-able component, like Analog.

I'm still unsure what we should do with a DontCare partial bundle in that case. Good options would be to interpret them as zeros, or treat them as nonliterals. Less-good options (correct but probably overly pedantic) might be to double-nest Options, or define some kind of type hierarchy encapsulating this.

Also worth considering is how to make this consistent with BitPat.

ducky64 commented 6 years ago

We discussed this at the meeting yesterday, and the resolution was that scenario 2 will be equivalent to scenario 3 (literals with a DontCare value will be treated as a non-literal type).

Other points discussed:

ducky64 commented 6 years ago

The yak shaving continues!

One issue is the use of _id, which does not produce useful results outside a Builder context. I think it would make sense to change it to a globally valid incrementing ID (does not reset between runs, so we'd want some kind of overflow detection - this could also mean long-running SBT instances would occasionally crash).

Note: the two current uses I see for _id are: