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

First-class enumerated signals #207

Closed
emilazy opened this issue Sep 14, 2019 · 22 comments
Closed

First-class enumerated signals #207

emilazy opened this issue Sep 14, 2019 · 22 comments
Labels
Milestone

Comments

@emilazy
Copy link
Contributor

emilazy commented Sep 14, 2019

#206:

we don't currently have anything like first-class enumerated signals

@whitequark
Copy link
Contributor

I'm not actually sure how to add these. You want to be able to do something like s == "FOO" but then they can't be Values without some real awful coercion going on.

@emilazy
Copy link
Contributor Author

emilazy commented Sep 14, 2019

Please don't use strings: https://docs.python.org/3/library/enum.html

@whitequark
Copy link
Contributor

OK, but if you define an IntEnum, you can use regular Signals. And also it would not help at all the FSM use case.

@emilazy
Copy link
Contributor Author

emilazy commented Sep 14, 2019

wish you had type inference of extensible variants ¯\_(ツ)_/¯

@whitequark
Copy link
Contributor

Wrong language for that.

@RobertBaruch
Copy link

RobertBaruch commented Sep 14, 2019

Here are my thoughts:

from nmigen import *
from nmigen.cli import main
from nmigen.asserts import *
from enum import Enum, unique

# Two different types of "enums".


class Thing(object):
    """A fake enumerated value.

    This isn't actually an Enum, but is just a bag of constants,
    and not at all Pythonic.
    """
    DOG = 0
    CAT = 1
    SQUIRREL = 2

    @classmethod
    def signal(cls):
        """Returns a Signal with the right number of bits for the Enum.

        src_loc_at tells nMigen which stack frame to get the variable
        name from, so the one above this one.
        """
        return Signal.range(0, 3, src_loc_at=1)


@unique
class ConstingEnumThing(Enum):
    """A real Enum.

    In general, we shouldn't care what the actual value of an enum
    is. If you do, you probably don't want an enum but a Const.

    We'll see later that the enum values don't matter. You can auto() them,
    or even use strings.
    """
    DOG = 0
    CAT = 1
    SQUIRREL = 2

    @classmethod
    def signal(cls):
        """Returns a Signal with the right number of bits for the Enum.

        src_loc_at tells nMigen which stack frame to get the variable
        name from, so the one above this one.
        """
        return Signal.range(0, 3, src_loc_at=1)

    @property
    def const(self):
        """Returns a Const with the right number of bits for the Enum.

        Although we use self.value, we'll see later on that we can
        dispense with that.
        """
        return Const(self.value, ConstingEnumThing.signal().shape())


class CatDetector(Elaboratable):
    """Uses Thing, which isn't an Enum.

    This issues a warning because width(Thing.CAT) = 1, and
    width(Thing.DOG) = 2.
    """

    def __init__(self):
        self.input = Thing.signal()
        self.output = Signal()

    def elaborate(self, platform):
        m = Module()
        self.detectCat(m, Thing.CAT)
        return m

    def detectCat(self, m, comparand):
        with m.Switch(comparand):
            with m.Case(Thing.DOG, Thing.SQUIRREL):
                m.d.comb += self.output.eq(0)
            with m.Default():
                m.d.comb += self.output.eq(1)


# This is the way we want things to work:
#
# But, we get an error:
# TypeError: Object '<ConstingEnumThing.CAT: 1>' is not an nMigen value
#
# class EnumCatDetector(Elaboratable):
#     def __init__(self):
#         self.input = ConstingEnumThing.signal()
#         self.output = Signal()

#     def elaborate(self, platform):
#         m = Module()
#         self.detectCat(m, ConstingEnumThing.CAT)
#         return m

#     def detectCat(self, m, comparand):
#         with m.Switch(comparand):
#             with m.Case(ConstingEnumThing.DOG,
#                         ConstingEnumThing.SQUIRREL):
#                 m.d.comb += self.output.eq(0)
#             with m.Default():
#                 m.d.comb += self.output.eq(1)


class EnumValueCatDetector(Elaboratable):
    """
    We can try to fix the error by using .value on all the enum values. But
    we run into the same width warning.
    """

    def __init__(self):
        self.input = ConstingEnumThing.signal()
        self.output = Signal()

    def elaborate(self, platform):
        m = Module()
        self.detectCat(m, ConstingEnumThing.CAT.value)
        return m

    def detectCat(self, m, comparand):
        with m.Switch(comparand):
            with m.Case(ConstingEnumThing.DOG.value,
                        ConstingEnumThing.SQUIRREL.value):
                m.d.comb += self.output.eq(0)
            with m.Default():
                m.d.comb += self.output.eq(1)


# We can't use the Const version on all the enum values, because:
#
# File "/home/robertbaruch/.local/lib/python3.6/site-packages/nmigen/hdl/dsl.py", line 286, in Case
#    switch_data["cases"][new_values] = self._statements
# TypeError: unhashable type: 'Const'
#
# class EnumConstCatDetector(Elaboratable):
#     def __init__(self):
#         self.input = ConstingEnumThing.signal()
#         self.output = Signal()

#     def elaborate(self, platform):
#         m = Module()
#         self.detectCat(m, ConstingEnumThing.CAT.const)
#         return m

#     def detectCat(self, m, comparand):
#         with m.Switch(comparand):
#             with m.Case(ConstingEnumThing.DOG.const, ConstingEnumThing.const):
#                 m.d.comb += self.output.eq(0)
#             with m.Default():
#                 m.d.comb += self.output.eq(1)


class EnumConstCatDetector(Elaboratable):
    """
    This is how you can use the Enum, but it's not great because
    now you have to remember when to use .value and when to use .const.

    And you still have to care about the actual numeric values of the Enum
    values, which isn't great.
    """

    def __init__(self):
        self.input = ConstingEnumThing.signal()
        self.output = Signal()

    def elaborate(self, platform):
        m = Module()
        self.detectCat(m, ConstingEnumThing.CAT.const)
        return m

    def detectCat(self, m, comparand):
        with m.Switch(comparand):
            with m.Case(ConstingEnumThing.DOG.value,
                        ConstingEnumThing.SQUIRREL.value):
                m.d.comb += self.output.eq(0)
            with m.Default():
                m.d.comb += self.output.eq(1)


def enumToConst(v):
    """Converts an Enum value to a Const of the correct size for the Enum.
    """
    assert (isinstance(v, Enum))
    s = Signal.range(0, len(type(v)))
    return Const(enumToValue(v), s.shape())


def enumToValue(v):
    """Converts an Enum value to a Value of the correct size for the Enum.

    Note that we do not care what the actual numeric values of the Enum
    values are. All we care about is that they are unique, start from 0,
    and monotonically increase by 1.

    O(N), but there's no reason you can't memoize a hashtable.
    """
    assert (isinstance(v, Enum))
    return list(type(v)).index(v)


def enumToSignal(t):
    """Converts an Enum type to a Signal of the correct size for the Enum.

    This could be useful if the Signal constructor accepts an Enum.
    """
    assert (issubclass(t, Enum))
    return Signal.range(0, len(t), src_loc_at=1)


class IdealEnumCatDetector(Elaboratable):
    """
    This is the ideal, if we imagine that Switch accepts Enum values and calls
    enumToConst() on them, Case accepts Enum values and calls enumToValue()
    on them, and Signal() accepts Enum classes and calls enumToSignal() on them.
    """

    def __init__(self):
        self.input = enumToSignal(ConstingEnumThing)
        self.output = Signal()

    def elaborate(self, platform):
        m = Module()
        self.detectCat(m, ConstingEnumThing.CAT)
        return m

    def detectCat(self, m, comparand):
        with m.Switch(enumToConst(comparand)):
            with m.Case(
                    enumToValue(ConstingEnumThing.DOG),
                    enumToValue(ConstingEnumThing.SQUIRREL)):
                m.d.comb += self.output.eq(0)
            with m.Default():
                m.d.comb += self.output.eq(1)


class OtherUses(Elaboratable):
    def __init__(self):
        self.input = enumToSignal(ConstingEnumThing)
        self.output1 = Signal()
        self.output2 = enumToSignal(ConstingEnumThing)
        self.output3 = Signal()

    def elaborate(self, platform):
        m = Module()
        with m.If(self.input == enumToValue(ConstingEnumThing.CAT)):
            m.d.comb += self.output1.eq(1)
        with m.Else():
            m.d.comb += self.output1.eq(0)

        # These should be disallowed, as should any math on enum values
        # except equality/inequality comparison.
        m.d.comb += self.output2.eq((self.input < 1) | (1 + self.input))

        # matches should allow Enums.
        m.d.comb += self.output3.eq(
            self.input.matches(enumToValue(ConstingEnumThing.CAT)))
        return m


if __name__ == "__main__":
    meow = CatDetector()
    # meow2 = EnumCatDetector()
    meow3 = EnumValueCatDetector()
    meow4 = EnumConstCatDetector()
    meow5 = IdealEnumCatDetector()
    meow6 = OtherUses()

    m = Module()
    m.submodules.meow = meow
    # m.submodules.meow2 = meow2
    m.submodules.meow3 = meow3
    m.submodules.meow4 = meow4
    m.submodules.meow5 = meow5
    m.submodules.meow6 = meow6

    main(
        m,
        ports=[
            meow.input,
            meow.output,
            # meow2.input, meow2.output,
            meow3.input,
            meow3.output,
            meow4.input,
            meow4.output,
            meow5.input,
            meow5.output,
            meow6.input,
            meow6.output1,
            meow6.output2,
            meow6.output3
        ])

@whitequark
Copy link
Contributor

Yeah, it's challenging to implement. Have you considered using an enum.IntEnum? That would work out of the box with nMigen.

@RobertBaruch
Copy link

IntEnum is frowned upon, mainly because it can be compared to other IntEnums (which enums shouldn't be) and other values in the same IntEnum, which is fine if you want that sort of thing.

For the majority of new code, Enum and Flag are strongly recommended, since IntEnum and IntFlag break some semantic promises of an enumeration (by being comparable to integers, and thus by transitivity to other unrelated enumerations). IntEnum and IntFlag should be used only in cases where Enum and Flag will not do; for example, when integer constants are replaced with enumerations, or for interoperability with other systems.

I'd prefer Enums over IntEnums where I need a signal whose values are orthogonal with all other values, can be compared for equality and assigned, but have no other defined numeric representation (except under implementation).

@whitequark
Copy link
Contributor

The way normal Enum works is at odds with nMigen's semantics, and I don't see any straightforward way to resolve that, so you're stuck working with the "frowned upon" mechanism right now.

@whitequark
Copy link
Contributor

Hmm... maybe Value.wrap could be taught to recognize enum.Enum?

@RobertBaruch
Copy link

RobertBaruch commented Sep 14, 2019

IntEnum seems to have the "you just have to know when to use .const" problem...

class IntThing(IntEnum):
    """An enumerated value based on IntEnum.
    """
    DOG = 0
    CAT = 1
    SQUIRREL = 2

    @classmethod
    def signal(cls):
        """Returns a Signal with the right number of bits for the Enum.

        src_loc_at tells nMigen which stack frame to get the variable
        name from, so the one above this one.
        """
        return Signal.range(0, 3, src_loc_at=1)

    @property
    def const(self):
        """Returns a Const with the right number of bits for the Enum.

        Although we use self.value, we'll see later on that we can
        dispense with that.
        """
        return Const(self.value, ConstingEnumThing.signal().shape())


class IntEnumCatDetector(Elaboratable):
    """CatDetector using IntEnum.

    You can dispense with .value, but you must still use .const.
    """

    def __init__(self):
        self.input = IntThing.signal()
        self.output = Signal()

    def elaborate(self, platform):
        m = Module()
        self.detectCat(m, IntThing.CAT.const)
        return m

    def detectCat(self, m, comparand):
        with m.Switch(comparand):
            with m.Case(IntThing.DOG, IntThing.SQUIRREL):
                m.d.comb += self.output.eq(0)
            with m.Default():
                m.d.comb += self.output.eq(1)

Without the .const:

elab.py:300: SyntaxWarning: Case value '10' is wider than test (which has width 1); comparison will never be true
  with m.Case(IntThing.DOG, IntThing.SQUIRREL):

@RobertBaruch
Copy link

Hmm... maybe Value.wrap could be taught to recognize enum.Enum?

It could, but then we'll have problems in Switch/Case with wideness warnings, no?

@whitequark
Copy link
Contributor

It could, but then we'll have problems in Switch/Case with wideness warnings, no?

Why? It could recognize Enum, check the maximum value, and emit a Const of an appropriate size.

@RobertBaruch
Copy link

It could, but then we'll have problems in Switch/Case with wideness warnings, no?

Why? It could recognize Enum, check the maximum value, and emit a Const of an appropriate size.

Yes -- that's the IdealEnumCatDetector case in my example. The idea is that enumToConst and enumToValue are the conversions that would be in nMigen.

@whitequark
Copy link
Contributor

@RobertBaruch So what do you think about adding (a) coerion from any Enum to a Const of an appropriate width, and (b) adding Signal.enum that creates a normal Signal from any Enum by interrogating it for minimum/maximum value, and checking that they're indeed integer?

@RobertBaruch
Copy link

I'm a fan! Anything would be better than having to code it up for each project :) As long as it works nicely with at least Switch/Case and If, I'd be happy. Signal.enum would be especially useful, and Enum->Const as well as Enum -> Value would be great.

@whitequark
Copy link
Contributor

Any Const is a Value already.

@RobertBaruch
Copy link

But then where does this error come from? If Enum becomes a Const, wouldn't that cause this error?

# We can't use the Const version on all the enum values, because:
#
# File "/home/robertbaruch/.local/lib/python3.6/site-packages/nmigen/hdl/dsl.py", line 286, in Case
#    switch_data["cases"][new_values] = self._statements
# TypeError: unhashable type: 'Const'
#
# class EnumConstCatDetector(Elaboratable):
#     def __init__(self):
#         self.input = ConstingEnumThing.signal()
#         self.output = Signal()

#     def elaborate(self, platform):
#         m = Module()
#         self.detectCat(m, ConstingEnumThing.CAT.const)
#         return m

#     def detectCat(self, m, comparand):
#         with m.Switch(comparand):
#             with m.Case(ConstingEnumThing.DOG.const, ConstingEnumThing.const):
#                 m.d.comb += self.output.eq(0)
#             with m.Default():
#                 m.d.comb += self.output.eq(1)

@whitequark
Copy link
Contributor

Ah, right. The matching code would have to be extended to handle Enum, too. But that's fine.

@RobertBaruch
Copy link

I can work on this, if you like.

@whitequark whitequark added this to the 0.1 milestone Sep 16, 2019
@RobertBaruch
Copy link

I'm VERY appreciative of this!

enums

@mithro
Copy link

mithro commented Sep 17, 2019

This is amazing! Great work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants