Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

schilkp
Copy link
Contributor

@schilkp schilkp commented Feb 7, 2025

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?

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.
@ericastor
Copy link
Collaborator

ericastor commented Feb 7, 2025

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
Copy link
Contributor Author

schilkp commented Feb 8, 2025

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? 1

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
Copy link
Collaborator

grebe commented Feb 10, 2025

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
Copy link
Collaborator

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
Copy link
Contributor Author

schilkp commented Feb 11, 2025

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
Copy link
Collaborator

That's correct!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants