Add initial support for slices

[leonardt]

The main motivation for this feature addition is to improve compiler performance. There's places in the magma code where we generate large bit-blasted connections for slices and we're hoping that by representing slices explicitly we can reduce the number of API calls (as well as internal logic that recurses over the bit-blasted select/connections). It should also improve readability of the code by collapsing the bit-blasted slices in the json and verilog.

This basic feature addition simply adds the notion of ":" in the select path to stand for slices. I'm open to alternatives, particularly if there's any issues with this syntax you forsee. I added minimal code to support this feature, which basically involved modifying canSel and sel to support the slice selstr, as well as the verilog backend to code generate the slice.

[leonardt]

An issue to consider:

What happens when we select/connect an array port that's already been connected in a slice? I'm not sure if/where coreir does this checking for normal array selects, but we should be sure to be consistent here and to catch this error. Fortunately for us, I don't know if this is a major user facing issue, since at the magma level we already do this checking, so at least in the magma->coreir flow we don't necessarily need this error checking except for validating the compiler internals (although this safety check would be nice for future development that might introduce bugs of this nature).

[leonardt]

Added some logic to verifyconnectivity to handle the slices (this was needed for magma support), for some reason I can't reproduce the magma issue for a test case, so working on that. Also, there might be a more clever (performant) way to handle skipping the indices, but this should work.

[rdaly525]

Looking through this now. I think I understand from a convenience point of view, but why not just leverage coreir.slice instances?

[rdaly525]

An issue to consider:

What happens when we select/connect an array port that's already been connected in a slice? I'm not sure if/where coreir does this checking for normal array selects, but we should be sure to be consistent here and to catch this error. Fortunately for us, I don't know if this is a major user facing issue, since at the magma level we already do this checking, so at least in the magma->coreir flow we don't necessarily need this error checking except for validating the compiler internals (although this safety check would be nice for future development that might introduce bugs of this nature).

This is done in the ModuleDef.validate method.

[rdaly525]

Some additional tests in order to add this feature: Does a slice work on any array type (or just arrays of bits)? Does slices of slices work correctly? Does selects of slices work correctly?

[leonardt]

@rdaly525 the main reason is performance, we want to avoid any extra logic in handling slices, so supporting them natively seems to make the most sense (since it's not too complicated) versus piggy backing on other mechanisms (which incurs more API calls to represent the same thing)

[leonardt]

For now I'm restricting slices only to work on array of bits, I think more general slices are harder to handle so let's start with the simpler case first (also will let us validate what kind of performance increases we can see). I can add a validator check to make sure we catch the error when slices are used otherwise. The slice of slices and select of slices tests are good, I'll look into them (but FWIW, magma won't use either of these so we could also make these errors for now)

[rdaly525]

Okay, as long as you catch and label those errors as NYI.

[rsetaluri]

re: arrays of things other than bits...it's not clear to me why this isn't supported automatically? is it a matter of the connectivity validation being harder?

[leonardt]

@rsetaluri hmm, I haven't though about it, I can take a look into how hard it would be to support. my initial thought was that since nested arrays are being flattened out anyways, we wouldn't be code generating these nested slices, so their representation is less helpful for our performance issue. In fact, I think this shows one place where complication might occur. When flattening a nested array, we would need to flatten the corresponding slices, which would be doable but likely a bit complex.

[rsetaluri]

Yeah that would need some extra logic - makes sense. Hopefully moving to N-D arrays in verilog will help.

[rdaly525]

Yeah a couple of passes might interact with this feature:

FlattenTypes RemoveBulkConnections PackBitConstants PackConnections UnpackConnections

Although really this illuminates the need for better testing on these already written passes.

[leonardt]

• FlattenTypes - is this problematic if we don't support general nested arrays? (Just arrays of bits) I can look into a test to see if it's a problem (perhaps the flattened connected wireables don't get updated properly, but I think my basic test uses nested arrays and it works fine).
• RemoveBulkConnections - I can update this to basically remove slices in a similar fashion to bulk connections, which should match the expected behavior
• PackBitConstants - this goes from 1-bit consts to a packed const, not sure if this is problematic unless we want the packed const to use a slice connection? Or is it if the consts are wired up to a slice?
• PackConnections, UnpackConnections - I'm not exactly sure what these do, are they related to remove bulk connections?

[rdaly525]

I think for a lot of these, just making sure the addPassthrough() function works, means that these should work.

• FlattenTypes - is this problematic if we don't support general nested arrays? (Just arrays of bits) I can look into a test to see if it's a problem (perhaps the flattened connected wireables don't get updated properly, but I think my basic test uses nested arrays and it works fine).

I suspect this will not need any updates till nd-array support

• RemoveBulkConnections - I can update this to basically remove slices in a similar fashion to bulk connections, which should match the expected behavior

This pass basically just transforms any circuit to that where connections are only bits or bitvectors. So, you should keep slices of bitvectors, but simplify any more complicated slices.

• PackBitConstants - this goes from 1-bit consts to a packed const, not sure if this is problematic unless we want the packed const to use a slice connection? Or is it if the consts are wired up to a slice?

Seems like no changes required.

• PackConnections, UnpackConnections - I'm not exactly sure what these do, are they related to remove bulk connections?

I think @dillonhuff wrote these passes. From what I can tell: PackConnections eagerly packs connections of selects of arrays into a single connection of that array if possible. Seems like this could be expanded to use slices. UnpackConnections does the opposite of this. Also could be expanded to remove uses of slices.

[rdaly525]

Other methods that come to mind that could be affected: Wireable.disconnectAll Wireable.getAllSelects Wireable.getLocalConnections

[leonardt]

I reviewed that code and I think because this just piggy backs on the existing connection/select logic (it doesn't introduce any new code paths there, it just adds a new path that can introduce a new select/connection that behaves just like others), so I think those should work as expected since there are no changes to the visible API for selects/connections.

Is there a pass I can use to test the use of these API calls? Maybe inlining a slice would be a good test

[leonardt]

It seems like I can use createcombview for the second two, and instancegraph or sanitize names for the first one, I'll try them out

[leonardt]

ok looks like disconnect all is used in detach field (instance graph node) which is used inside flatten types, so the basic test involves wiring slices inside a nested type that is flattened so that seems to work as expected.

[leonardt]

Tried out comb view with slices connected to/from a register and seems to work as expected