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

Hwtypes2 #515

Merged
merged 127 commits into from Jan 10, 2020
Merged

Hwtypes2 #515

merged 127 commits into from Jan 10, 2020

Conversation

leonardt
Copy link
Collaborator

Mantle branch: phanrahan/mantle#165
Fault branch: leonardt/fault#187

Migration guide is in docs/magma_2_migration_guide.md (we'll probably need to add to this)

@leonardt
Copy link
Collaborator Author

leonardt commented Jan 9, 2020

Question on the relationship between directed/undirected types.

Right now, a directed type is not related to an undirected type, e.g.

>>> isinstance(m.In(m.Bit)(), m.Bit)
False
>>> isinstance(m.In(m.Bit), m.BitKind)
True

This works because we use instances of the meta class to check the type of the type, so the type of directed types are the same (they are instances of BitKind)

However, now we are planning to use issubclass(T0, T1) to check type relation.

For this to work, we need m.In(m.Bit) to be a subclass of m.Bit, so we can check

>>> issubclass(m.In(m.Bit), m.Bit)
True

But that also means that a directed instance is an instance of an undirected instance

>>> isinstance(m.In(m.Bit)(), m.Bit)
True

Which is a departure from the old behavior.

I don't think this should affect existing code since usually in this case we were using the meta class relationship.

The question is: is having an instance of an directed type also an instance of an undirected type a reasonable design? Otherwise we may have to revert to the isinstance of meta class relationship we have been using before.

@phanrahan
Copy link
Owner

I think the old way was broken, and the new way is better. That is, a directed type should be a subclass of an undirected type.

@leonardt
Copy link
Collaborator Author

leonardt commented Jan 9, 2020

Good to hear :), I discussed this offline with James H. and Raj and they both agree.

@leonardt
Copy link
Collaborator Author

I've added tests for the various typing relations, I'll merge this in tomorrow (alongside mantle/fault) unless there are any more objections. Peak/lassen/garnet will need to be updated

@phanrahan
Copy link
Owner

Looks great. Thanks for adding all these tests.

@splhack
Copy link
Contributor

splhack commented Jan 10, 2020

This code works with master

from magma import *
from mantle import *

def test_namedtuple():
    class A(Product):
        a0 = Bit
        a1 = SInt[8]

    @circuit.sequential(async_reset=False)
    class TestNamedTuple:
        def __init__(self):
            self.a0: Bit = bit(0)
            self.a1: SInt[8] = 0

        def __call__(self, a: A, b: Bit) -> A:
            if b:
                new_a = a
            else:
                new_a = namedtuple(a0=self.a0, a1=self.a1)

            self.a0 = new_a.a0
            self.a1 = new_a.a1

            return new_a

test_namedtuple()

but got error with hwtypes2

Traceback (most recent call last):
  File "magma/magma/ast_utils.py", line 54, in compile_function_to_file
    exec(code, defn_env)
  File ".magma/TestNamedTuple_comb.py", line 5, in <module>
    class TestNamedTuple_comb(m.Circuit):
  File "magma/magma/circuit.py", line 441, in __new__
    self.definition()
  File ".magma/TestNamedTuple_comb.py", line 13, in definition
    new_a_2 = phi([new_a_1, new_a_0], io.b)
  File "mantle/mantle/common/operator.py", line 74, in wrapper
    retval = fn(*args, **kwargs)
  File "mantle/mantle/common/operator.py", line 296, in mux
    return Mux(len(I), T=T, **kwargs)(*I, S)
  File "mantle/mantle/coreir/MUX.py", line 126, in Mux
    return DefineMux(height, width, T=T)(**kwargs)
  File "mantle/mantle/coreir/MUX.py", line 89, in DefineMux
    class _Mux(Circuit):
  File "magma/magma/circuit.py", line 417, in __new__
    self = CircuitKind.__new__(metacls, name, bases, dct)
  File "magma/magma/circuit.py", line 101, in __new__
    renamed_ports=dct["renamed_ports"])
  File "magma/magma/interface.py", line 185, in __init__
    port = port.flip()
  File "magma/magma/tuple.py", line 374, in flip
    cls.__name__, (base, ), {})
  File "hwtypes/hwtypes/adt_meta.py", line 128, in _cache_handler
    t = mcs._from_fields(fields, name, bases, ns, **kwargs)
  File "magma/magma/tuple.py", line 345, in _from_fields
    t = TupleKind.__new__(mcs, name, bases, ns, **kwargs)
  File "magma/magma/tuple.py", line 41, in __new__
    raise TypeError("Can't inherit from multiple different bound_types")
TypeError: Can't inherit from multiple different bound_types

looks like due to In vs Out?

bound_types=(Out(Bit), Out(SInt[8]))
base.fields=(In(Bit), In(SInt[8]))

--
By the way, is there a way to create instance from class A(Product) aside from namedtuple?
something like

a = A(a0=self.a0, a1=self.a1)

@leonardt
Copy link
Collaborator Author

@splhack thanks, fixed by c3fa956

@leonardt leonardt merged commit d97e35c into master Jan 10, 2020
@leonardt leonardt deleted the hwtypes2 branch January 10, 2020 21:59
@splhack splhack mentioned this pull request Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants