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.

[quark17]

The message is about the definition for out in the module (at line 10, though referring to the use of y at line 11). However, what is happening under the covers is that BSC is creating an interface struct from all the definitions, and the names for the fields of that struct are taken from the interface type declaration, and out is declared at line 2. So that causes an error on out in the module to end up pointing to out in the type. It's a known issue that we should fix at some point (Bluespec Inc internal bug database issue 1238).

However, it's interesting that your PR has changed the position from the name (out) to the type (Bit#(8)). It could be due to how you're picking positions for any ISyntax/CSyntax constructs that you're building, or maybe how you've changed the expression structures (causing getPosition to find a different position first). I'll try to keep an eye out for it while looking through your changes. (I do think it's worth understanding why positions change -- regardless of what we think of the specific test example, because it could show up as a position change in other situations that we're not testing.)

[krame505]

I spent a while looking at this, and I'm having a hard time figuring out what is going on here. As far as I can tell, this id is coming from the name in the FInf built by chkInterface, which has the type as its position. I'm not sure how this previously had a different position.

[quark17]

Note that the testcase is compiled with -cross-info. Without that flag, the message doesn't display a position. So it's something in the -cross-info mechanism that is inserting the better position (and maybe the default behavior, without the flag, hasn't changed).

This is a flag/mechanism that I would like to excise, and replace with good position tracking by default. Given that, I think it's OK to not worry about the change now. If it's a problem, we can fix it later.

However, if you feel like looking to see how the -cross-info flag was working before, and whether that position could be preserved somehow now (either with or without the flag), that'd be welcome.

Maybe we should at least add a second text to the exp file that compares the output of the test when compiled without the flag, too, just to check for regressions in that behavior, too.

[krame505]

I don't really expect to have time to keep investigating this soon, but I added a test to compare the output without --cross-info.

[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).

[quark17]

Hm, would the size of (x,()) be reported as 1? Do we think it should be 2?

[krame505]

Hmm, I suppose it would be reported as 1.

I don't think it's too unreasonable to treat tuples that end in () as one element smaller. Note that AppendTuple also would drop the () if the first tuple being appended ends in (), and the same for CurryN.

[quark17]

I recall that there is a general issue with tuples: because they are build from nested pairs, the last element can't be of a tuple type, otherwise it's indistinguishable from one large tuple. For example (and this works in BSV too), if you have a variable of type Tuple2 a (Tuple2 b c), you can assign to it with tuple3 -- and the current implementation of TupleSize considers it 3. Here's code that you can try:

package TupleSizeBH where

type T3 = Tuple2 Bool (Tuple2 Bool Bool)

mkTupleSizeBH :: (TupleSize T3 3) => Module Empty
mkTupleSizeBH =
  module
    let x :: T3
        x = tuple3 True False True

However, if we have a variable of type Tuple3 a b (), that cannot be assigned with tuple2 -- BSC gives a type error unless you use tuple3. However TupleSize says its 2. This inconsistency seems wrong to me.

The issue is that the expression(e1,e2) parses into PrimPair e1 e2, and (e) parses the same as e, and () parses into PrimUnit. Longer tuples are nestings of PrimPair, but PrimUnit is not a zero of PrimPair -- this is perhaps clearer in BSV, where you can't write (), it has to be written as void.

The inconsistency can be fixed for TupleSize by defining it like this:

instance TupleSize a 1 where {}
instance TupleSize (a,b) 2 where {}
instance (TupleSize b n) => TupleSize (a, b) (TAdd n 1) where {}

But you're saying that AppendTuple and curryN also use PrimUnit as a zero. Here is an example showing that appending a Tuple3 and Tuple2 will give Tuple4 if the Tuple3 ends with PrimUnit:

package AppendTupleBH where

type TA = Tuple3 Bool Bool ()

type TB = Tuple2 Bool Bool

-- type TC = tuple5 Bool Bool () Bool Bool
type TC = Tuple4 Bool Bool Bool Bool

mkAppendTupleBH :: (AppendTuple TA TB TC) => Module Empty
mkAppendTupleBH =
  module
     let x :: TA
         x = tuple3 True False ()
         y :: TB
         y = tuple2 True False
         z :: TC
         z = appendTuple x y

This seems to be because you want to support () as a zero in calls to appendTuple and splitTuple, and so there need to be instances for (), but you have also written your typeclass so that it reuses the top-level instances for sub-elements of the tuple. If you separate the processing of the sub-elements from the top-level, I think the issue goes away. Of course, there's also a complication that BSC won't allow overlapping instance of the form T () x and T x (), even if a more explicit instance for the intersection T () () is given -- which is unfortunate, and maybe something that can be fixed one day? You got around that by adding a second level of typeclass. However, that second level is still conflating top-level () with sub-element (). I think you just need to add a third level of typeclass, that's only for processing once top-level () has been eliminated -- and in fact I confirmed that this works:

class AppendTuple a b c | a b -> c where
  appendTuple :: a -> b -> c
  splitTuple :: c -> (a, b)

instance AppendTuple a () a where
  appendTuple x _ = x
  splitTuple x = (x, ())

instance (AppendTuple' a b c) => AppendTuple a b c where
  appendTuple = appendTuple'
  splitTuple = splitTuple'

class AppendTuple' a b c | a b -> c where
  appendTuple' :: a -> b -> c
  splitTuple' :: c -> (a, b)

instance AppendTuple' () a a where
  appendTuple' _ x = x
  splitTuple' x = ((), x)

instance (AppendTuple'' a b c) => AppendTuple' a b c where
  appendTuple' = appendTuple''
  splitTuple' = splitTuple''

class AppendTuple'' a b c | a b -> c where
  appendTuple'' :: a -> b -> c
  splitTuple'' :: c -> (a, b)

-- Top-level () has been handled before AppendTuple''
-- so no instance is needed for () because occurrences
-- can only be sub-elements at this point and are handled
-- by the instance for sub-elements of any type

instance AppendTuple'' a b (a, b) where
  appendTuple'' a b = (a, b)
  splitTuple'' = id

instance (AppendTuple'' a b c) => AppendTuple'' (h, a) b (h, c) where
  appendTuple'' (x, y) z = (x, appendTuple'' y z)
  splitTuple'' (x, y) = case splitTuple'' y of
    (w, z) -> ((x, w), z)

I haven't looked closely at curryN, but I assume that a layer of hierarchy could be added, with () not handled in the second layer.

[quark17]

I was worried that there's still the issue that splitTuple of Tuple3 Bool Bool () to split off the last element and then appendTuple to put it back won't result in the original tuple. But actually, both definitions are allowed -- by your implementation and mine! Unless I'm missing some reason why this example compiles:

package TestSplitAppend where

type TA = Tuple3 Bool Bool ()

mkTestSplitAppend :: Module Empty
mkTestSplitAppend =
  module
     let x :: TA
         x = tuple3 True False ()
         p :: ((Bool,Bool), ())
         p = splitTuple x
         y1 :: Tuple2 Bool Bool
         y1 = case p of
                (a,b) -> appendTuple a b
         y2 :: TA
         y2 = case p of
                (a,b) -> appendTuple a b

     let e1 :: (Bool, Bool)
         e1 = (True, False)
         e2 :: ()
         e2 = ()
         e31 :: (Bool, Bool)
         e31 = appendTuple e1 e2
         e32 :: (Bool, Bool, ())
         e32 = appendTuple e1 e2

And it's not because the types are considered the same, because adding y1 == y2 causes a type error.

There's a dependency on the typeclass, so for the same arguments, only one result type should be possible:

class AppendTuple a b c | a b -> c where

Separately, isn't there something missing in the dependencies you declared? Shouldn't it also have a c -> b and b c -> a?

[krame505]

I'm just getting back to your comments here - I'm not sure I agree your version of TupleSize is what we want?
The version you specified would have TupleSize () 1, but we want () to have size 0 for the purposes of checkPortNames. I suppose we could introduce TupleSize' and make (a, ()) have size 2 but () have size 0 - would that address your concern?

[krame505]

I updated the TupleSize, AppendTuple and Curry type classes with an additional level of indirection to avoid treating (a, ()) as a tuple of size 1. Adding the extra reverse functional dependencies creates some ambigous instance errors, so I did not do that.

[krame505]

To be specific, adding the extra fundeps on AppendTuple and its helpers gives

Error: "Prelude.bs", line 3449, column 9: (T0128)
  Overlapping typeclass instances Prelude.AppendTuple'' a b (a, b) and
  Prelude.AppendTuple'' (h, a) b (h, c) (defined at "Prelude.bs", line 3453,
  column 34 ) cannot be ordered from most-specific to least-specific, so they
  cannot support predictable instance-resolution. Please resolve this by
  changing one or both of the instances.

[quark17]

The message is about the definition for out in the module (at line 10, though referring to the use of y at line 11). However, what is happening under the covers is that BSC is creating an interface struct from all the definitions, and the names for the fields of that struct are taken from the interface type declaration, and out is declared at line 2. So that causes an error on out in the module to end up pointing to out in the type. It's a known issue that we should fix at some point (Bluespec Inc internal bug database issue 1238).

However, it's interesting that your PR has changed the position from the name (out) to the type (Bit#(8)). It could be due to how you're picking positions for any ISyntax/CSyntax constructs that you're building, or maybe how you've changed the expression structures (causing getPosition to find a different position first). I'll try to keep an eye out for it while looking through your changes. (I do think it's worth understanding why positions change -- regardless of what we think of the specific test example, because it could show up as a position change in other situations that we're not testing.)

[quark17]

• I'd like to investigate if the interface on vMkRWire can be removed (eliminating all the changes needed there) and then the exporting of primMethod won't be needed.
• I'd like to understand why id_to_vName in VModInfo.hs needs to be changed to remove the qualifier -- you said it may have had to do with vMkRWire, so maybe that change goes away if we remove that.

I barely remember what was going on with vMkRWire now, other than that it seemed very hard to fix nicely. What do you mean by "the interface on vMkRWire"? If that module exists, it needs to have an interface? I do see that the numeric parameter on VRWireN isn't needed, since it is only ever 1, is that what you meant?

Sorry, I meant to say vMkRWire1. The discussion about it was here. The interface is not needed because the module should never be called by a user -- it only exists so that BSC can construct the AVInst for an RWire of size 1, and to do that BSC runs some of the compiler stages on vMkRWire1 to compile it and then extract the AVInst of the submodule _rw, throwing away everything else, including the interface of the parent module. My suspicion was that we could change vMkRWire1 to have an Empty interface, and in fact, I tried that and it works. So I'm going to push a commit that cleans this up. (Someday, we could clean further by eliminating vMkRWire1 and having BSC construct the AVinst in some better way.) FYI, the tests for BSC's use of this are in bsc.scheduler/conflict_free/.

It may be worth preserving that commit in the history, so maybe we submit it as a separate PR and then rebase this PR on it when merged?

[quark17]

I've reviewed all your recent additions, now. Almost there! Thanks!

There are a couple previous issues remaining, but they can probably stay unresolved in this PR, or maybe I'll have a closer look when I can. I'll post an updated list of those issues (sometime later).

[krame505]

@quark17 I believe I have addressed all your feedback. Is this ready to merge now?

[quark17]

I think it could be ready! There are some things that I could submit as separate PRs first, and then rebase, unless that would be disruptive:

1. I can submit the change to vMkRWire1 as a separate PR, so it's preserved in the commit history.
2. I can submit a PR to make tests not sensitive to changes in PreludeBSV position (bsc.names/signal_names/ and bsc.bluetcl/commands/).

Otherwise, the remaining issues can be dealt with in the next PR or later (and by someone else):

3. The positions in name-clash error messages (G0055) have changed

• But this didn't result in any change in the testsuite, because we don't check for it! There are a couple tests for the error code in bsc.names/portRenaming/{ready,enable}Tests/ and a ton in bsc.names/portRenaming/conflicts/*/ but they don't examine the output, so didn't notice that the position changed. I'd like us to change those tests (or some of those tests) so that they do compare the position. It'd be best if we could do that as a prior PR, so that it's visible what changes in this PR. But I don't know how much work it would be to update all those conflicts/*/ tests. (Or we could just change a few representative examples?)
• In any case, it would be nice to improve the positions. It's not obvious how, and I don't want to hold you up, so I'll open a GitHub Issue to record what we know. See thread.

4. The next PR will improve the messages in bsc.verilog/noinline/, but there's still extra context reduction messages, arising from earlyContextReduction not being thorough -- I'll submit a GitHub Issue for that (which doesn't have to be dealt by you in these PRs). See thread.

5. The position changed in bsc.mcd/Misc/ClockCheckCond.bsv, when using the -cross-info flag. This was already an issue that needed improvement (see Issue Clock-crossing errors should have better positions without the -cross-info flag #863). It's not obvious how to get the previous position, and it shouldn't hold you up, so I've just recorded the issue with a comment on 863 (rather than open a new Issue). See thread.

6. The fact that AppendTuple can only have dependencies in the forward direction is bugging me. (See thread.) You're making heavy use of it and it seems to be fine, so I'm OK deferring it. I might open a GitHub Issue, just to record it. Also, I'm suddenly wondering whether incoherent is needed on the AppendTuple declaration and whether that would change anything?

7. Overall documentation for the feature and for SplitPorts, will be needed, but can come later.

8. The new test in bsc.codegen/vector_interfaces/ used -dtypecheck to dump the wrapper in the second time the typecheck stage occurs. When PR Some miscellaneous improvements to dump flags #859 is merged (improving how dumping works), the flag here may change. (I'm not sure which will get merged first, as 859 is being re-thought.)

Anyway, I assume we'll hold off on merging this until the follow-on PR is close to ready to merge? I'm OK with merging this ahead of time, if that's easier on you, but if so, let's do it after the pending release.

Is the follow-on PR still in progress? Should I start reviewing it?

[krame505]

I think it could be ready! There are some things that I could submit as separate PRs first, and then rebase, unless that would be disruptive:

1. I can submit the change to vMkRWire1 as a separate PR, so it's preserved in the commit history.
2. I can submit a PR to make tests not sensitive to changes in PreludeBSV position (bsc.names/signal_names/ and bsc.bluetcl/commands/).

If you would like to make those changes as separate PRs and rebase, that would be fine.

Otherwise, the remaining issues can be dealt with in the next PR or later (and by someone else):

Thank you. I don't know if/when I will have time to look at the position changes.

8. The new test in bsc.codegen/vector_interfaces/ used -dtypecheck to dump the wrapper in the second time the typecheck stage occurs. When PR Some miscellaneous improvements to dump flags #859 is merged (improving how dumping works), the flag here may change. (I'm not sure which will get merged first, as 859 is being re-thought.)

No preference what order those are merged, it is easy enough to change.

Anyway, I assume we'll hold off on merging this until the follow-on PR is close to ready to merge? I'm OK with merging this ahead of time, if that's easier on you, but if so, let's do it after the pending release.

Is the follow-on PR still in progress? Should I start reviewing it?

The follow-on PR is functionally complete from my end, @nanavati hasn't had a chance to review it yet though, and I'm not sure when he will.

If this PR is merged before reviewing the output port splitting changes, then that review can just happen here in #849. Otherwise you will need to review it in krame505#1, which is based on the genwrap branch in my fork.