[xls][mlir] Support XLS fifo properties/config in MLIR channels.

[schilkp]

As discussed via email.

Support XLS fifo properties/config in MLIR channels.

This also adds support for converting these properties to/from XLS, and updates different passes (array_to_bits, index_type_conversion) to ensure these attributes are maintained when channels are modified.

Finally, these properties are also exposed for sproc channels, and the proc elaboration pass is updated to assign said properties to all eproc channels that are generated from an sproc channel.

To be discussed: Are the input/output flopping fifo configuration values required?

[ericastor]

FWIW, the I/O flopping configuration values are important to many FIFO libraries - they change the way the FIFOs are instantiated. You can see one example of how it's relevant here, in our own FIFO generator: https://github.com/google/xls/blob/main/xls/codegen/materialize_fifos_pass.cc#L162

They don't need to be in v1 of this, but they should probably be added at some point! (EDIT: And if they are here, they should almost certainly stay.)

[schilkp]

They don't need to be in v1 of this, but they should probably be added at some point! (EDIT: And if they are here, they should almost certainly stay.)

Got it - I was not sure so I implemented them while I was at it so they are good to go :)

FWIW, the I/O flopping configuration values are important to many FIFO libraries - they change the way the FIFOs are instantiated. You can see one example of how it's relevant here, in our own FIFO generator: https://github.com/google/xls/blob/main/xls/codegen/materialize_fifos_pass.cc#L162

Just for my own understanding - I was digging around this part of codegen (also for #1896) and I still don't quite understand how input_flop_kind and output_flop_kind (the two pieces of ChannelConfig that are outside of FifoConfig) are used in the materialize FIFOs pass.

As far as I can tell only FifoConfig is used to construct the fifo because that pass does not have access to the original channel's ChannelConfig (in other words, it uses depth, bypass, register_push_outputs, and register_pop_outputs only)

Are those values somehow constructed from input_flop_kind and output_flop_kind?

The only other reference I can find to those values is during the "Convert IR to Blocks" pass, which seems to construct registers on the proc blocks the channels are attached to? ¹

I am asking because I have been working on a project that relies on precisely how XLS instantiates buffers, so I am trying to make sure I really understand it :)

Footnotes

1. https://github.com/google/xls/blob/1d94eb0a27491db56174cb40cc18fe3e5332c8dd/xls/codegen/proc_block_conversion.cc#L1324 ↩

[grebe]

I believe ChannelConfig holds the metadata for both internal (FIFO-based) and boundary channels. It might be more accurate for its constructor arg to be a std::variant<FifoConfig, FlopKind, FlopKind>.

This is why the materialize_fifo_pass doesn't deal with FlopKinds- it only concerns itself with FIFO configurations, as FIFOs are used for internal channels, not boundary channels. The pass doesn't need access to the whole ChannelConfig b/c the FifoConfig is the only relevant portion.

[ericastor]

Another way to think of it - the FIFO config describes the FIFO that you'll need to instantiate if the channel is internal (between two XLS procs, or from a proc back to itself). The rest of the ChannelConfig describes how the proc will interact with the channel, regardless of what the FIFO actually is... e.g., whether to insert registers on the proc side of the I/O path, and how.

We've had a lot of confusion around channels because of this naming issue - in different contexts, "a channel" can refer to any of:

1. the bundle of ports & protocol that a proc needs to use to talk to an external entity,
2. the logic that actually connects one set of ports to another (usually a FIFO), and
3. the collection of everything on the connecting path between procs (including the ports on both ends & the logic between them).

I've been starting to think we should have disambiguating names for these.

[schilkp]

Thanks you two for the explaination. That really clears it up. I was very confused why the flopping was not relevant for the fifo instantiation directly.

To make sure I get this right:

For input(output) boundary channels:

• The input(output) flopping determines if the connected proc input(output) gets a flop/buffer.
• The fifo config is ignored.

For internal channels:

• The input+output flopping determines if the connected proc input and output gets a flop/buffer
• The fifo config is used to parametrize the actually FIFO that makes up the channel.

In other words - the input and output flopping is relevant for both internal and boundary channels?

[ericastor]

That's correct!

[schilkp]

rebased.

[jpienaar]

Nice, sorry got delayed with CGO/C4ML prep. Looks good and nice mapping retaining the core info. One could also do a roundtrip test (in one lit test convert back and forth and diff) to easily test both paths.

[schilkp]

Thanks for the review :)Traveling today, will get to this tomorrow.

One could also do a roundtrip test (in one lit test convert back and forth and diff) to easily test both paths.

Is there a LIT-specific mechanism for this, or would you just do an // RUN: That pipes between translation tools into filecheck, with a number of // CHECK:s that validate nothing changed?

[schilkp]

@jpienaar feedback addressed except for one point I would @jmolloy's feedback on :)

Edit: Maybe you could ping him, not sure if my ping-ing gets through ;)

[jpienaar]

Is there a LIT-specific mechanism for this, or would you just do an // RUN: That pipes between translation tools into filecheck, with a number of // CHECK:s that validate nothing changed?

Pretty much the latter yes. One can diff with the output of opt tool which would strip the comments (and then you don't need explicit CHECK lines), but often just checking some local invariant is fine for the lit one (it at least verifies nothing failed and specific information retained). Having a more complete diff tool that understands what can safely be ignored is better, and then could be run uniformly over all tests even.

[jpienaar]

Looks good to me, thanks!

[jpienaar]

Approving again as seems workflows didn't start (else I may manually go and poke)

[schilkp]

Ah I was quickly writing the round-trip test if you want to hold off for another 20min or so :)

I ping you when done.

[schilkp]

Ah I was quickly writing the round-trip test if you want to hold off for another 20min or so :)

I ping you when done.

@jpienaar Actually it seems that nobody every did a MLIR -> IR -> MLIR roundtrip and this is broken because of some loc-related bugs. I have it mostly fixed but won't wrap it up today. Feel free to merge this and I open a PR for the location-based bugs and round-trip test of fifo props tomorrow, or feel free to hold off merging this :)

[jpienaar]

I'll submit this one. For locations: remember to enable printing locations, else parsing will go south/drop it :)