Support splitting up struct method parameters into multiple input ports

[krame505]

After some back and forth with @nanavati, we ended up going with a design that is closer to my original proposal in #714. It is really hard to do port splitting in GenWrap.hs with the degree of flexibility that we want, and there are some limitations like not being able to resolve numeric type operations, e.g. for computing the size of a vector. Not to mention that code is incredibly tedious and hard to modify, and would only make it harder to implement features like methods with multiple output ports. Doing this with type classes makes the logic a bit more transparent and easier to modify in the future.

There are several significant changes bundled together in this pull request:

1. A significant refactor to wrapper generation, using a type class to determine the types of fields in wrapper interfaces, replacing some of the hard-coded logic in GenWrap.hs;
2. Move port type saving logic out of the evaluator and genwrap into the type class;
3. Pass method argument names by tagging method fields with a new primitive instead of via BetterInfo (this allows determining input port names at elaboration time);
4. Move the sanity checks for port name collisions to post-elaboration;
5. Determine how method arguments are split into ports using a type class;
6. Add some port splitting utilities using generics.

Implementation

I'm not changing the fundamental structure of wrapper interfaces, only changing how field types are determined - flattening of nested interfaces still happens in genwrap as usual. The types of fields in a wrapper interface are determined by the typeclass WrapField. The toWrapField method converts from a field in the original interface (e.g. Int 8 -> ActionValue Bool) to the type of field in the wrapper interface (e.g. Bit 8 -> ActionValue_1), while fromWrapField is the inverse of this. The special cases for Clock, Reset and Inout are also handled by WrapField.

WrapField uses a type class WrapMethod to compute the wrapped type of a method field. This type class uses the SplitPorts type class to convert the type of a method input into a tuple of Ports, and the WrapPorts type class converts this into a tuple of Bit values. (Thus one determines how a method argument type should be split into ports by defining instances of SplitPorts. More on that later.) The individual tuples of method argument Bit values then get turned into a single curried function type for the wrapper interface method.

These type classes are also used to compute the names of input ports. This is happening on the value level as lists of strings¹, not as type-level strings as I was originally thinking. I hit some sort of snag with this, although I don't remember exactly what it was, and being able to just compute this in the evaluator seemed like less of a pain than dealing with type-level lists of strings, adding type-level number to type-level-string conversion, etc.² The only reason a type-level string is there in WrapField is to give the original field name to generate a better error message when context resolution fails due to a type not being in the Bits type class.

The list of argument names are tagged on to the wrapper method function/value with primMethod. The evaluator now expects this primitive to exist on method fields in iExpandField.³ We could potentially stick additional metadata that is computed at elaboration-time here in the future. The field name/result pragma and the arg_names pragma (if present) are passed as arguments to toWrapField, which are used to compute the base names of input ports, which are and tagged on to the converted method value.

Because port names are now determined at elaboration time, I had to move the port name collision checks to after elaboration. This is maybe slightly less nice as some error messages show up latter, but this sort of error isn't super common. It does feel like a more natural place to implement these checks anyway, instead of needing to figure out the port names from the pragmas before type checking.

Saving port types, on both sides of the synthesis boundary, is also handled via these type classes. See the saveFieldPortTypes method in WrapField type class. Calls to this method get inserted in both genwrap and wrappergen. This method also requires the same field naming arguments as toWrapField. I considered making toWrapperField/fromWrapperField be in the Module monad and do the port type saving too, but that complicates the code generation in genwrap a fair bit as every field value needs to be bound in a giant do-block.

Specifying port splitting

How a method argument type gets split up, and how the resulting ports are named is determined by the SplitPorts type class. There is a default instance that doesn't do any flattening, which preserves the current behavior:

instance SplitPorts a (Port a) where
  splitPorts = Port
  unsplitPorts (Port a) = a
  portNames _  base = Cons base Nil

If we have a struct

struct Bar =
  v :: Vector 3 Bool
  w :: (Bool, UInt 16)
  z :: Foo

interface Top = 
  putBar :: Bar -> Action

then for putBar to have separate input ports for each field, we need an instance

instance SplitPorts Bar (Port (Vector 3 Bool), Port (Bool, UInt 16), Port Foo) where
  splitPorts (Bar { v = v; w = w; z = z; }) = (Port v, Port w, Port z)
  unsplitPorts (Port v, Port w, Port z) = Bar { v = v; w = w; z = z; }
  portNames _ base = Cons (base +++ "_v") $ Cons (base +++ "_w") $ Cons (base +++ "_z") Nil

One can write this sort of instance explicitly. However there are a few ways that this can be done with less boilerplate.

I added a library SplitPorts in Base1 with a couple of utility type classes. ShallowSplitPorts uses generics to flatten out a struct by one level, using the SplitPorts instances for each of its fields. One can use these to define a SplitPorts instance:

instance (ShallowSplitPorts Bar p) => SplitPorts Bar p where
  splitPorts = shallowSplitPorts
  unsplitPorts = shallowUnsplitPorts
  portNames = shallowSplitPortNames

This would be a bit nicer to use if we had deriving via. In fact, I'm wondering if we should make derive SplitPorts generate the above sort of instance automatically.

DeepSplitPorts fully flattens a struct, including nested struct, tuple and Vector⁴ fields, down to primitives and types with multiple constructors. When using this type class, if one wishes for some nested struct type not to be flattened, they can define a DeepSplitPorts instance that does nothing to prevent this.

Sometimes one might wish for a type to be flattened in only some places. Instead of defining a SplitPorts instance, you can insert the ShallowSplit or DeepSplit "newtype" wrapper on your interface method parameters:

interface Top = 
  putBar :: DeepSplit Bar -> Action

I added test cases illustrating all these different patterns/approaches. There are probably more possibilities and I'm not sure what will prove to be the most ergonomic in practice, but these utilities are easy to add/change later.

Future considerations

I designed this with support for methods with multiple output ports in mind, which I may or may not attempt next depending on how much time I have. The SplitPorts type class could be reused to also determine how results of value/ActionValue methods are split into output ports.

I'm not quite sure what the wrapper type representation looks like for types with multiple output ports. Just using a tuple of Bit values for methods with multiple output ports might work for value methods, but ActionValue_ only accepts a single numeric size parameter. My current thinking is that we should ditch ActionValue_ and have
a struct PrimValue :: (# -> * -> *) n a that tags a Bit n value onto a chain of output values a, ending in a PrimAction or PrimUnit.

Remaining issues

The error message when a method yields a port that isn't in Bits is fine, but there is another error message about a Bits context that didn't reduce, with unknown position. See for instance testsuite/bsc.verilog/noinline/NoInline_ArgNotInBits.bsv.bsc-vcomp-out.expected. I'm not totally sure where this is coming from or how to suppress it, but it maybe isn't too bad.

Congrats on making it to the end of this wall of text. Hopefully @quark17 has time to look this over before the sync meeting on Friday?

Footnotes

1. Really, this should be using ListN to ensure that the list of port names is always the correct length. But sadly that doesn't exist in the Prelude, and SplitPorts needs to be. ↩

2. Although in retrospect the various tuple shenanigans I needed were just as complicated, and I could maybe have just stuck the port name in the Port type constructor. So I'm not sure if it ended up being much simpler. Having the evaluator is more flexible, at least. ↩

3. This required a corresponding tweak in vMkRWire1, which is a handwritten interface LARPing as a generated wrapper interface, to be instantiated way later by the scheduler. ↩

4. Making this work reasonably for large vectors required a fun bit of awesomeness in the ConcatTuple type class I added, which converts a vector of tuples to and from a flattened tuple. ↩

[nanavati]

Do you know why this position changed?

[krame505]

I'm not sure I understand why this error message had the position that it did in the first place. It is still pointing to the same interface field at least.

[nanavati]

I think it would be good to have tests for the error messages you get if SplitPorts instances are wrong (wrong number of names, name conflicts between generated ports and any others you can think of).

[nanavati]

Why did you need a qualified version of idCons? Are the existing uses of idCons wrong?

[krame505]

Yeah the existing idCons is unqualified for some reason. I am comparing equality with the id coming out of ICon, which is qualified. I suppose I could just compare the base string but comparing with the whole id seems better?