Strong enums

[hngenc]

This is an implementation of the strongly-typed enums proposed in #885. It satisfies all the conditions laid out there, along with these extra features:

• UInts can be cast into enums
• The user can retrieve a list of all valid enum values through the StrongEnum.all function

Caveats

The enums do have two primary caveats that should be considered:

• An enum class must be "static" in the Java sense. In other words, you can't define an enum class inside of another class, although you can define it globally, or in an object, as in this example:

object FSM { // The companion object to a module named FSM
    class State extends EnumType
    object State extends StrongEnum[EnumType] {
        val s1, s2 = Value
    }
}

• Casting non-literal UInts to enums is an inherently dangerous procedure, because Chisel has no way of knowing whether or not the UInt is a valid enum value. When this happens, we emit warnings, but @jackkoenig has suggested completely removing this feature unless users demand its inclusion. Needless to say, casting literal UInts to enums is safe, and provides compile-time guarantees.

API Modifications

• In the master branch, Element inherits from Data and override's Data's def width with a val width in the constructor. I removed this constructor parameter from Element, so that EnumType could inherit from Element without having to specify the width directly in the constructor (since EnumType's width isn't actually known until all EnumTypes have been instantiated). Now, other subclasses of Element, like Bits and Clock, must explicitly override Data's width with their own implementations (which I have already taken care of).
• Numerous functions, like switch, is and bindLitArg require parameters of type Bits in the master branch. However, since EnumType must also work with these functions, and since EnumType isn't a subclass of Bits, I modified these functions so that they instead take in parameters of type Element.

Neither of these API modifications should break any existing user code. All the old tests still pass.

Related issue: #885

Type of change: feature request

Impact: API modification

Development Phase: implementation

Release Notes

• Added support for strongly-typed, self-annotating enums

[edwardcwang]

With regards to "Casting a non-literal UInt to an enum" --> what if this casting operation returned a tuple of the enum and a valid bit?

i.e. as follows:

val myUInt: UInt = io.myInput
val (asEnum, valid) = MyEnumClass.fromUInt(myUInt)

Then if myUInt was not a valid Enum value, then the valid signal is now, and asEnum is DontCare or other undefined behavior.

[hngenc]

@edwardcwang The problem is that we would need to insert circuitry that constantly monitors the value of myUInt to make sure that it is valid. This seems wasteful to me, especially in real hardware. SystemVerilog has an assert property feature that would fix this problem, but that would only be useful in simulation and not in real hardware.

[edwardcwang]

Well, if we don't use the valid signal, most synthesis tools will prune it away. And in fact even if we generate the signal in Chisel but don't do anything else with it, I think FIRRTL's optimization passes will trim it away. If we were really desperate for it not to show up in Chisel, we could even do something like:

val asEnum = MyEnumClass.fromUIntRaw(myUInt)
// or
val (asEnum, unused) = MyEnumClass.fromUInt(myUInt, raw=true)

Honestly, I would just recommend generating both and letting FIRRTL/synthesis tools prune away unused logic. Having this extra option could be a good balance between safety and performance (if you care about safety or if fault tolerance is important, then use valid; else just ignore valid).

[hngenc]

@edwardcwang That makes sense. I'll look into implementing this feature. It should be pretty easy.

[albert-magyar]

Why not just add an assertion instead of a valid signal? That makes it easier to use and automates the simulation-only checking.

[aswaterman]

It might legitimately be invalid on some clock cycles, so you'd have to gate the assertion.

[albert-magyar]

True!

[hngenc]

OK, I've added an isValid boolean signal to all enums. I've also renamed the function that casts from non-literals to castFromNonLit and added the following restriction:

• Non-literals being cast to enums cannot be wider than the enum itself

I suppose if users want, they can assert(enum.isValid).

[chick]

I think you should change the test
test/scala/cookbook/FSM.scala
to use your new Enums, people look at the tests to
see the right way to do something

[jackkoenig]

Overall this looks amazing, great work. I have some comments, suggestions, etc.

One specific thing is I am wondering how we should think about inheritance with enumerations; not because I think it should be allowed, but this makes it kind of look like you can use it. The following leads to an error:

  import scala.reflect.ClassTag
  abstract class Parent[T <: EnumType : ClassTag] extends StrongEnum[T] {                                                                                                         
    val e0, e1, e2 = Value
  }
  class Bar extends EnumType
  object Bar extends Parent[Bar] {
    val e3 = Value
  }

[error] Caused by: chisel3.core.EnumExceptions$IllegalDefinitionOfEnumException: chiselTests.JackTest$Bar defined illegally. Did you forget to call Value when defining a new enum
?
[error]         at chisel3.core.StrongEnum.<init>(StrongEnum.scala:336)

I suspect inheritance should just not be allowed but just wanted to bring this up.

[jackkoenig]

@ducky64 Could we use this mechanism for Bundle field ordering? I suspect Scala reflection was too flakey back in the day for this to work, but maybe it does now?

[ducky64]

Yes, there were cases where Scala reflection just pukes all over the place. I think this had to do with inner classes and fun like that, so we're still stuck with looking at id order, which is kind of hacky and nasty.
We use Scala reflection in autoclonetype, but that's fine because that's best effort - it's more or less accepted to tell the user that they need to overload cloneType (though we might not have if we were doing clean-slate design).

[hngenc]

@jackkoenig Your comments sound good and I can start implementing them. However, last night, I discovered the magic of macro annotations, and I wrote an (ugly) macro that converts this:

@ChiselEnum
object State {
    val sIdle, s1, s2 = Value
}

to something that behaves just like

class State extends EnumType
object State extends StrongEnum[State] {
    val sIdle, s1, s2 = Value
}

I'm thinking of changing this to the official API, since it reduces boilerplate. The disadvantage is that IntelliJ won't be able to provide users with code completion in this case, but it may still be worth it. I also think I might be able to remove the "only static enums" limitation if we use this annotation instead.

Do you (or anyone else) have any thoughts on this? Does it look elegant to you?

Edit: I can confirm that macro annotations remove the "only static" restriction. In some ways, they make the code uglier, but in others, they make it cleaner. In particular, the recursive constructor will be no longer be needed.

[ducky64]

Expanding the use of annotations is interesting, though probably warrants discussion. The current place where annotations are used is only in chiselName, which doesn't affect the generated logic. There is also discussion to use annotations for templating the do_* functions, clone module (advanced user feature), and bundle literals (which potentially could see wide adoption, though the annotation is optional - and one downside is that people might define a bundle without using the annotation, or that the literal constructors might seem magic to those who don't read the docs)

[hngenc]

Sorry for doing this after my code had already been reviewed, but I've changed the API again, one last time:

object State extends ChiselEnum {
    val sIdle, s1, s2 = Value
}

This is just as concise as Scala Enumerations, and preserves type-safety. Additionally, with this API, enums do not have to be static.

Macro annotations were fun, but IntelliJ can't handle them, so I got rid of them.

[ccelio]

Thanks for keeping IntelliJ in mind!

[hngenc]

The new enums work by creating a different ChiselEnum.Type for each enum you declare.

Here's how they can be used (it's nearly the same as the old version):

object State extends ChiselEnum {
    val sIdle, s1, s2 = Value
}

val state = RegInit(sIdle)

// or, alternatively:
val state = Reg(State())
state := sIdle

// or, alternatively:
val state = Reg(State.Type())
state := sIdle

There is one downside to this implementation, which is shared by Scala enumerations:

def foo(e: MyEnum.Type) = ???
def foo(e: OtherEnum.Type) = ??? // This causes a compiler error

Basically, you can't overload functions with different types of enums as parameters.

However, it's important to note that type-safety is preserved in the following sense:

def foo(e: MyEnum.Type) = ???

foo (MyEnum.s1)
foo (OtherEnum.sOther) // This will cause a compiler error

So during compilation, Scala will prevent users from passing in the wrong types of enums to functions.

Personally, I don't think there are going to be many use cases where users want to overload functions that take enums as parameters, so I think the reduced verbosity of this new style is worth it.

[jackkoenig]

This looks awesome, great work!

@chick can you re-review?

Should we start out with this in experimental @ducky64?

[chick]

It all looks good to me. My only concern is the change to Elements signature could have upstream consequences. I am not sure if the testing on this branch has run the chisel-testers test suite with this branch. If it has then I will approve.

[ducky64]

Some comments about the implementation.
I second @jackkoenig - we should expose this in chisel3.experimental instead of the base chisel3 package.

[ducky64]

Should probably have some documentation, preferably ScalaDoc

[chick]

@hngenc can you update the branch to up to date with master and this can be merged.