Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Statically usable literals #417

Closed
ducky64 opened this issue Dec 16, 2016 · 15 comments
Closed

Statically usable literals #417

ducky64 opened this issue Dec 16, 2016 · 15 comments
Labels
Feature New feature, will be included in release notes
Milestone

Comments

@ducky64
Copy link
Contributor

ducky64 commented Dec 16, 2016

Currently, once you create a UInt literal, you can't get the literal back out. For RTL construction purposes, this isn't much of a issue (since you don't usually need it back), but because Chisel's literal from string constructor supports more radix types than base Scala (notably, binary), it helps to be able to use the same syntax for example when writing testvectors.
One possible proposal is for UIntLit to extend UInt, where UIntLit has an additional method to get the value. UIntLit would be returned by all literal constructors.

@aswaterman
Copy link
Member

Chisel2 had isLit and litOf methods defined on Data (or maybe Bits?). The API wasn't ideal, but the feature was a useful feature from time to time. It was also even useful for RTL, as sometimes an implementation can be improved on the basis of constness. It's the same rationale behind gcc offering __builtin_constant_p.

@sdtwigg
Copy link
Contributor

sdtwigg commented Dec 16, 2016

For constructing binary and hexadecimal numbers succinctly, you can just have a StringContext for them, e.g.: h"abcd" or b"1111" that returns a BigInt.

Similarly, you could add an inspection function to see if some UInt or SInt is currently bound to being a literal. (It is really odd how Data currently has the literal methods.)

@ducky64
Copy link
Contributor Author

ducky64 commented Dec 16, 2016

So the StringContext is a method, as would be PML on strings (i.e. "1111".b), but Chisel needs a standardized way to do this on tests, otherwise everyone else is going to write their own and it's going to be a giant mess. I think the most intuitive API would allow these to be specified the same way as Chisel literals (where we can do like "b111".U). Considering that bundle literals are also a desired feature, it should interoperate with that too, when (if?) it happens.

@sdtwigg
Copy link
Contributor

sdtwigg commented Dec 16, 2016

No, you should actually read the documentation: http://www.scala-lang.org/api/2.12.0/scala/StringContext.html

You can define a StringContext such that b"1111" translates to BigInt(15) (or, likely more literally, BigInt("1111", 2)) so your tests have immediate access to the BigInt

@ducky64
Copy link
Contributor Author

ducky64 commented Dec 16, 2016

I know how StringContext works (b"1111"). PML ("1111".b) was another way that's been commonly suggested. Yes, they're different (I assume this is what you mean).

For the purposes of this discussion, what I want to resolve is standardizing literal specification in a way that's accessible for a testbench, and in a way that would both be consistent with how RTL literals are currently specified ("b1111".U) as well as compatible with potentially upcoming features (like Bundle literals).

@sdtwigg
Copy link
Contributor

sdtwigg commented Dec 16, 2016

The UIntLit is somewhat problematic because you will be unable to define a proper cloneType method for it. Path dependent types like this.type are checked by scala but the JVM will still require UIntLit.cloneType return a UIntLit. However, the contract of cloneType requires that return item be bindable to anything so Reg(init = 0.U) will rapidly result in a UIntLit representing a register.

I don't understand the target scenario. It seems odd that you would start with a scala numeral, transform it into a Chisel literal, and then want the scala numeral back (but have no way of structuring the code that you can retain both).

In general, I think it is an odd (likely bad design) to start with a scala numeral, transform it into a Chisel literal, and then want the scala numeral back. If a testbench needs (and knows it will need) as both scala numeral like BigInt, Int, etc. (e.g for functional modelling) and Chisel literal, then it should generate, store, and manipulate scala numerals right up until it has to inject the value into the Chisel dut (done via a literal creation).

A UInt => BigInt function because it makes a false promise. (Even UInt => Option[BigInt] will end up being None a bit too often.) I think it would be a bad idea to make it too easy for users to expect a BigInt to be extractable from something of type 'UInt'. (All it takes is one designer writing testbench having a helper function that takes a literal UInt and another designing expecting to be able to pass anything there. Further, this would expose a major (somewhat implementation-particulars difference) between storing your data in Seq(0.U, 1.U, 2.U) and Vec(0.U, 1.U, 2.U) and then doing a static lookup.

There are a few rare times it may be exceptionally convenient to try to extract a scala numeral from a Chisel.UInt (like from an enumeration). Even then, it is rarer that you want to actually treat the enumeration numerically.

Thus, making it a reflection-style function (so isolated in its own subpackage with a name reinforcing its volatile nature) should help prevent the user from considering it part of the core API to be used extensively. In fact, we used to have a handful of testbenches that needed this function (although at the time I just used litValue defined on Data) but, after a few rounds of refactoring and cleanup, we were able to remove their use completely.

@ducky64
Copy link
Contributor Author

ducky64 commented Dec 16, 2016

Yeah, Scala value -> Chisel lit -> Scala value obviously sounds pretty awful in the case of numbers, though if we have Bundle literal constructors it's less clear how that should be handled (separate method call for literals? perhaps defaulting construction to Scala-land values, with a .toLit or something to create a Chisel-land literal object? perhaps two constructor signatures, one accepting Chisel-land types and another accepting Scala-land types?).

I was hoping for UIntLit, though I see the issue with cloneType now. I also remember that there was a discussion a while ago about this.type being bad practice, though I don't think there was a good resolution. F-bounded polymorphism seems like it would allow better control over cloneType, though it incurs a pretty nasty syntax cost for Bundles.

I also agree that UInt => BigInt is a bad idea.

I'd still like thoughts on a literal specifier for Scala-land values that has syntax consistent with Chisel literal specifiers. Perhaps something like "b1111".L (which returns a BigInt, but shares parsing logic with .U and friends)? Though reserving .L might be too limiting of the future? Or perhaps removing deprecating the PML string literal constructor and restructuring it as a chain of StringContext and literal conversion (like b"1111".U)?

I think we can also do a better job of enums (#373), rather than just having them return Chisel types. One option may be to have enum values be objects that have methods to get as either a BigInt or Chisel literal.

@ducky64
Copy link
Contributor Author

ducky64 commented Jan 13, 2017

I think it's time we made another push for this, while simultaneously considering implications for #418 Bundle literals and #373 Enums.

Considering integer literal types alone, either StringContext (u"0b111") or PML ("0b111".L) are clean and safe - it's just a discussion about style (which is still very important). The overall philosophy would be "construct literal types in Scala-land, and do a cast to Chisel-land", much like how you start with 2, then do a .U conversion to turn it into a UInt literal.

A similar philosophy may work with Enums, where they could be used as Scala-land integers and converted to Chisel-land through a .U conversion. The main benefits from using a custom Chisel Enum type would be ability to propagate value names down to Verilog (future feature as this isn't yet supported anywhere in the Chisel stack) and consistent bitwidths of all values in the same Enum. What I would like to see would be (though I don't yet know if the type system / syntax will support it):

object MyState extends ChiselEnum
object MyStateIdle extends MyState.Value(0)
object MyStateRunning extends MyState.Value(1)

in RTL:

val currState = Reg(MyState.asType)
currState := MyStateIdle.U

in test:

poke(currState, MyStateIdle)

However, where this would get hairy would be how to make it type-safe (allowing APIs that are more specific than any ol' UInt) and perhaps how to separate Scala-land and Chisel-land types (since we're only declaring a single Chisel-land ChiselEnum type). If more types need to be declared, macro metaprogramming may be part of the solution to eliminate the need to write repeated boilerplate.

A similar problem would come up with Bundle constructors. Macro metaprogramming would make it possible to automatically generate a companion object and literal constructor for Bundle types, but the problem is that Bundle types are Chisel types rather than Scala types. For example this:

@bundle
class MyInnerBundle extends Bundle {
  val a = UInt(2.W)
  val b = UInt(4.W)
}
@bundle
class MyBundle extends Bundle {
  val a = UInt(6.W)
  val z = new MyInnerBundle()
}

would expand with companion object constructors with signatures:

MyInnerBundle(a: UInt, b: UInt) -> MyInnerBundle
MyBundle(a: UInt, z: MyInnerBundle) -> MyBundle

side note: it's almost always undesirable to construct a Bundle literal in both directions, so more thought about that is needed; the example above sidesteps it as a undirectioned bundle

but the problem is that since it takes a UInt, it will not be safe to turn those back into Scala-land types for use in testing. One possible nasty solution is to understand the Scala-land / Chisel-land mapping and automatically generate literal constructors instead:

MyInnerBundle(a: BigInt, b: BigInt) -> MyInnerBundleLit
MyBundle(a: BigInt, z: MyInnerBundleLit) -> MyBundleLit

but this is probably really, really brittle and doesn't extend / compose well (this function would need to be kept up-to-date with all Data subtypes). It's also generating a different MyBundleLit type in the background, which could confuse people trying to debug code ("where did this class come from and why can't I find it in my source?").

The problem with both Bundle and Enum literals seem similar (Scala-land and Chisel-land data types without requiring user boilerplate and in a type-safe manner) and a good solution will probably apply across both.

I think that in the larger picture, we need a Chisel-land data type, but don't necessarily need a Scala-land data type (for instance, if given guarantees, preferably statically checked, that a Chisel-land data type could be turned into a integer, it wouldn't be terrible API design to take a UInt in a test function) - though the problem with this path is how to get those guarantees.

@azidar
Copy link
Contributor

azidar commented Jan 13, 2017

My vote is "0b111".U to get UIntLiteral(7), and "0b111".S to get SIntLiteral(-1). This seems the most similar to the existing mechanism.

Re: the enum proposal - I can't think of any way to test whether a returned value value is 0, or MyStateIdle, unless types propagate to Firrtl. I say we table this in favor of other immediate concerns.

Re: literal bundles - I have a feeling that adding an @bundle macro is probably a bad idea from a usability perspective. If people forget it, we get a crazy error. In contrast, we could have a dynamically-checked constructor that takes a map. Though not statically checked, we could through a very clear error message, and have more context into what the user was intending to do.

@ducky64
Copy link
Contributor Author

ducky64 commented Jan 13, 2017

We can table the discussions of what a Enum constructor could do, but the resolution to the literal specifications will restrict the range of (philosophically consistent, at least) solutions for Enums literals and Bundle literals. Which is why I'd like to have a resolution that makes everything possible, even if we don't decide on the exact semantics and implementation of those yet.

For Enums, one immediate gain would be type safety, for example assigning a general UInt to a Enum wire would error out.

I don't particularly like dynamic Map-based Bundle literal constructors, both because the type signature isn't meaningful (a good IDE should be able to tell you exactly what the fields are for a macro-based version; whereas a generic Map is meaningless) and there's clunky map syntax (as opposed to named arguments). I'm on the fence about the macro doing too much behind the scenes, though it's arguable that there's precedent with case classes that automatically generate a constructor companion object for you.

@ducky64
Copy link
Contributor Author

ducky64 commented Jan 13, 2017

Also, see Stephen's post above about why UIntLiteral (as a separate class, which both extends UInt but also gives literal conversion guarantees) would be problematic (mostly that all Data-derived subtypes define cloneType that returns its own type, so UIntLiteral cloneType must return UIntLiteral, which is not generally useful). There may be ways around that that are worth discussing (for example, can UIntLiteral be made not a subtype of UInt while still supporting the use cases we care about?).

In response to @sdtwigg 's comment about Scala value -> Chisel lit -> Scala value sounding bad, one possibility is to redefine the testing APIs to take Chisel lits rather than Scala lits (so you avoid that last conversion, at least as far as the user is concerned)

@sdtwigg
Copy link
Contributor

sdtwigg commented Jan 13, 2017

One thing to note is that stuff like b"1111" or h"ffff" (returning UInt) will end up syntax highlighting nicely (where just the literal part is red rather than the radix selector).

@ducky64
Copy link
Contributor Author

ducky64 commented Jan 24, 2017

Random thought for statically usable literals:
What about replacing Data.cloneType: this.type with object cloneType and def apply[T <: Data](toClone: T): T = .... Then, a DataLiteral (or something) could be mixed in to a UInt and provide some conversion function (like toBigIntBits) on the literal (or not, even just marking it as a literal would be fine). This should provide a correct return type (similarly to how Wire(...) and Reg(...) work; unlike this.type which isn't strictly correct) and a correct API.

Internally, it would call Data.cloneType: Data (and let's be blunt, we're throwing around .asInstanceOf[this.type] everywhere and losing type-checker guarantees anyways).

Of course, there would need to be some interop (so we may leave Data.cloneType: this.type alone, define a new Data.newCloneType: Data and have the default implementation delegate to the old one so old code still compiles). If may also be possible to add some syntactic sugar (for example, implicit conversions that give the illusion of Data.cloneType (and you'll never chain an apply after a non-synthesizable node anyways) but properly type-parameterized.

@sdtwigg ?

@ducky64
Copy link
Contributor Author

ducky64 commented Feb 6, 2017

^^ this implementation strategy for typeclassed cloneType was in #193

@ducky64 ducky64 added this to the 3.1.0 milestone Apr 18, 2017
@ducky64 ducky64 added the Feature New feature, will be included in release notes label Nov 8, 2017
@ducky64 ducky64 modified the milestones: 3.1.0, 3.2.0 Dec 13, 2017
@ducky64 ducky64 mentioned this issue Feb 16, 2018
9 tasks
@ducky64
Copy link
Contributor Author

ducky64 commented Feb 16, 2018

Superseded by #777

@ducky64 ducky64 closed this as completed Feb 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature, will be included in release notes
Projects
None yet
Development

No branches or pull requests

4 participants