Prototype combinational2/sequential2 interface

[leonardt]

Here's an initial proposal for the new sequential syntax.

It becomes a sub-class of Generator2, which allows the user to declare self.io and instances inside the init method. This doesn't change anything, so effectively now allows init to be used to generate circuits. call is a special method that is rewritten using ast_tools ssa (to implement combinational semantics) and then is invoked with self (reference to generated circuit, so it can use things like instances of registers) and then the members of self.io that match the signature (we can improve this interface, but for now, the user has to declare them in two places, once in self.io and once in call, but call doesn't need a type since self.io has the type).

I think there's one major interface issue to resolve:

call now has different semantics depending on sequential versus normal circuit. normally it will wire up all inputs and return all outputs, in sequential here, call can specify a partial function from inputs to puts (the rest is defined in init). Ideally I'd like to make them consistent, so our options would be to have call satisfy the general circuit interface (declare all inputs/outputs even if some are unused), or use a different method for sequential (e.g. transition_function or update) to distinguish it from the call interface. The issue with this is it doesn't map directly to peak anymore, which uses the call interface, but perhaps that's not a problem since in peak the call interface should equal the update/transitionfunc interface, so it could just be a renaming

[leonardt]

Here's another proposal: we can have call method declare IO as before, and init can append new ports to the IO if desired, however this requires us to use a different code path than Generator2 since it doesn't follow this pattern, but it should use similar logic. This would then allow Peak to use the new version without any changes.

[leonardt]

Actually, the problem with the above method boils down to the parametrized signature. If we use init for generator parameters, then we can't declare types in call that refer to parameters, so we would have to use some other pattern for generators (e.g. functions), but I'm wary of introducing multiple ways to write generators (Our hope is to have the Generator2 pattern be the standard moving forward).

Thoughts on an alternative syntax to Peak for declaring types using parameters from init? I know @rdaly525 desired init generator parameters but ran into this exact same issue.

[rsetaluri]

(Ignore what you saw before, didn't realize it was a prototype.)

I also agree it would be nice if we could unify the interfaces for generators and for peak such that we don't have to have a custom meta class here etc

[cdonovick]

How I would deal with the partial function / double declartion of IO. Would be to have the IO inferred from the __call__ annotations. And just inject that in to the class namespace e.g:

class Foo(sequential):
    def __call__(self, x : T, y : S) -> R:
        ...
# becomes
class Foo(circuit):
    io = ('x', In(T), 'y', In(S), 'out', Out(R)) 
     # I don't know what the correct syntax is now but my intention should be clear
    def __call__(self, x : T, y : S) -> R:
        ...

I definitely don't want to declare the io in init.

We should use https://docs.python.org/3/library/inspect.html#inspect.Signature.bind for building the params.

[cdonovick]

Also preferably it would not rely on a metaclass. I would like to be able to use it in peak but if the metaclass adds a bunch of magma specific stuff it makes it much harder to use.

[rdaly525]

Thoughts on an alternative syntax to Peak for declaring types using parameters from init? I know @rdaly525 desired init generator parameters but ran into this exact same issue.

Just to be clear, I was actually advocating passing in parameters that are semantically equivalent to CoreIR's ModuleArgs. These are arguments that specifically do not affect the type of the circuit and therefore I consider I different semantically from generator parameters. Things like register init values or constant values. But importantly the typing of these parameters (how many bits are in the register init value) could need to be derived from passed in generator arguments.

[rdaly525]

I think the main issue with this proposal is it fundamentally mixes the concept of a generator with the concept of a circuit. I would expect something that does something along the lines of:

class MyGen(Generator):
    def gen_circuit_type(self, gen_arg1, gen_arg2):
        #returns the concrete circuit type
        InT0 = f0(gen_arg1, gen_arg2)
        InT1 = f1(gen_arg1, gen_arg2)
        InT2 = f2(gen_arg1, gen_arg2)
        OutT = f2(gen_arg1, gen_arg2)
        return m.IO(in0=inT0, in1=InT1, in2=InT2, out=OutT)

    def gen_circuit_instances(self, gen_arg1, gen_arg2):
        #creates the list of instances used in the circuit

    def gen_init_parameters(self, gen_arg1, gen_arg2):
        #create parameters to be passed into circuit.sequential.__init__ 

    def gen_circuit_call(self, gen_arg1, gen_arg2):
        T0, T1, T2, OutT = self.gen_circuit_type(gen_arg1, gen_arg2).values()
        def call(self, in0: T0, in1: T1, in2: T2) -> OutT:
            #same semantics as circuit.sequential.__call__

        return call

Obviously the syntax/structure/naming should be better to make it easier for users, but keeping a real distinction between the generator stage and the circuit stage seems important. For example, call annotations are not known until you pass in generator parameters and therefore it cannot be a method on the generator class and does not really make senes to be a method on the generator class. There should also be the ability to have a materialize() function that takes in generator parameters and exactly produces the old circuit.sequential class. Obviously I have a CoreIR-centric bias here, but just my two cents

[leonardt]

I think if we separate the notion of a circuit and generator to be different classes, we have a problem with unifying sequential and generators, because sequential requires a class definition (to separate init from call), so either the user has to define a metaclass (generator for sequential class) or we have to use a different pattern for generating sequential, e.g. function factory generates class.

I don't think having the user specify a meta class is really in the cards, so either we need to mix the two syntaxes in the same class, or we have to revert to using the function factory generates sequential, which diverges from the m.circuit/m.generator pattern.

[leonardt]

Another option would be have to __call__ defined inside the m.Generator.__init__ and have the user monkey patch (we can provide an API for this that also adds the ports to IO) it onto the circuit class they are creating. That way they have access to the generator parameters for the type signature.

[leonardt]

    class Basic(m.Generator2):

         def __init__(self, width):
             super().__init__()


             self.io = m.IO(opcode=m.In(m.UInt[3])) + m.ClockIO(has_ce=True)
             self.x = Register(width, has_ce=True)
             self.y = Register(width, has_ce=True)
             self.x.CE @= self.io.opcode >= 2
             self.y.CE @= self.io.opcode == 3

             def __call__(self, I: m.In(m.UInt[3]) -> m.Out(m.Bits[width]):
                  # TODO: Register assign syntax, e.g. self.y = self.x
                  return self.y(self.x(I))

             self.add_sequential_call(__call__, output_name="out")  # appends ports to IO

perhaps this also resolves the issue with generic __call__ for wiring and __call__ for sequential since this adds the explicit notion that this method is for sequential logic.

[rdaly525]

Another option would be have to __call__ defined inside the m.Generator.__init__ and have the user monkey patch (we can provide an API for this that also adds the ports to IO) it onto the circuit class they are creating. That way they have access to the generator parameters for the type signature.

I think this is the way to go. In some sense I think of a magma generator class as just syntax convenience for writing:

def myGenerator(arg0, arg1, arg2):
   @sequential:
   class MyCircuit:
      #...

[leonardt]

Using __call__ defined inside __init__ would also remove the dependency on a sequential meta class (the logic would be hidden inside the add_sequential_method API), however the sequential meta class logic is also trivially implemented in a decorator so that's not a huge concern.

[leonardt]

For Caleb's use case where he doesn't want to declare io inside __init__, we can also expose the add_sequential_method (or provide an @sequential2 decorator) for methods inside a regular m.Circuit, so this would use the standard io class syntax and append the call ports to the current io object.

[leonardt]

I'll see how this looks sketching it out

[leonardt]

@rdaly525 yes, that's the intention of the syntax, it's also meant to standardize things like parameter handling and provide common re-useable infrastructure by leveraging class patterns. but at it's core it's really just a framework for doing the old function style generator syntax with more structure.

[rdaly525]

Why get rid of the old sequential (which represents a circuit) then? Why not just build this generator_sequential as a different abstraction opposed to one that replaces it? You can still advocate users to use this new abstraction while allowing peak to just build upon the old one.

[rdaly525]

That would solve caleb' issue about the init and call now having different semantics. init and call of sequential_generator is just a new magma-only thing that has different semantics and is unrelated to the init and call of circuit.sequential

[leonardt]

Hmm that's an option, I'll see how that works out.

[leonardt]

@cdonovick I've updated the implementation to greatly simplify. It doesn't introduce any semantic changes to sequential, instead it uses annotations to avoid exec, so the only exec is in the ssa transformation of call. It also updates call to use the ast_tools ssa, I will work on combinational in a separate PR, but I don't think that will affect you.

Can you check it out and maybe test it on some simpler circuits? I'll have to get it up to feature completeness with the existing test suite, but perhaps there are only a subset of features you need to get started working with it.

The overview of the new algorithm:

• run ast_tools.ssa on cls.call
• inspect cls.call.annotations to build IO
• generate class definition with IO, invoke call with the IO attributes for the corresponding input ports, wire up the return of call to the output port(s)

[cdonovick]

@leonardt current code structure looks great! Exactly what I had in mind.

The SSA pass will be probably need to be updated to better handle calls, I can take that on.

[leonardt]

I hacked in an implementation of the register assign syntax using a monkey patched setattr, there's probably a better way to implement this using a descriptor, but we need some notion of a magma register primitive where we would define this. Right now the register is an ad-hoc user defined circuit (in mantle) so in general it's hard for us to detect whether the attribute is a register versus some other circuit. I think for now this should work with existing code (modulo minor bugs), but I will look into moving the register into magma as a primitive so we can use some more principled logic in handling them

[leonardt]

Hmm, I think something like @apply_rewrites(passes=[m.combinational]) would be nice. It makes composing with something like loop_unroll more explicit (they're both AST transformation passes, unrolling has to happen before combinational), but it only requires one decorator so it's not as verbose as the begin/end rewrite pattern.

[leonardt]

It also makes it more clear that m.combinational is doing some rewrite magic

[splhack]

sequential2 with explicit CLK #524 (comment)

[leonardt]

@cdonovick hmm, I was looking at this more. We can't quite just drop combinational2 into a pass since it does more than rewrite the tree (it wraps the result of the rewrite in a circuit object to implement the magma interface).

So, we would need some way to have an "end_rewrite hook" (e.g. define a method that is invoked after all the rewrites are done). Not sure if there's a reasonable pattern for extending the apply_passes interface to this?

Basically we need to: (1) rewrite the AST (standard apply passes pattern), (2) wrap the result of the rewrites in an object.

My hope is to have the user only write one decorator (the above could be easily handled by a second decorator that consumed the result of the passes). In the current implementation, magma strips the combinational decorator, runs the passes, then wraps the result of the passes. Perhaps this isn't such an unreasonable design? The main issue is just the stripping of the combinational decorator, which will be hard in general but should work fine in the basic case that most users will be doing (e.g. @m.combinational2)

[cdonovick]

@leonardt Would the following not work?

class comb2(apply_ast_passes):
   def __init__(self):
         super().__init__(passes=passes_comb2_runs)

   def exec(self, *args, **kwargs):
       fn = super().exec(*args, **kwargs)
       # post_rewrite_stuff 

[cdonovick]

apply_passes has 3 hooks btw:

• parse
• strip_decorators
• exec

https://github.com/leonardt/ast_tools/blob/master/ast_tools/passes/util.py#L157-L188

[cdonovick]

We could add another if you need it but I think exec is what you are looking for

[leonardt]

That seems like it could work, I'll try it out

[leonardt]

That worked, depends on leonardt/ast_tools#49 though

[cdonovick]

LGTM. My only request would be for a hook to add pre/post comb passes (both in seq and in comp) but we can handle that in a different PR if you want.

[leonardt]

What would the interface look like? E.g. we could add an argument to comb/seq so something like

m.combinational(pre_passes=[..], post_passes=[...])

[cdonovick]

That seems good. I didn't have any specific interface interface in mind.

[leonardt]

I'm going to merge this since I think it's been through enough discussion/iteration. Let's try using it and see how it goes and manage issues in follow up PRs