llvm / circt

Circuit IR Compilers and Tools
https://circt.org
Other
1.67k stars 298 forks source link

[Handshake] Deprecating the FIRRTL lowering #5102

Closed mortbopet closed 1 year ago

mortbopet commented 1 year ago

Deprecating the Handshake-to-FIRRTL lowering has been a long-standing to-do for many people, and for many reasons:

In the near future, I'll most-likely be doing a refactoring of the handshake dialect to remove the notion of "every SSA value is a handshaking connection". It was a neat idea, but in reality, it makes it impossible/really cumbersome to compose handshake and non-handshake code. Instead, i'd like to introduce a handshake type + a new operation handshake.unit which can wrap any other operation with unit-rate semantics (essentially the only "benefit" of the "every SSA value is a handshake" semantic).

Handshake-to-HW is almost finished, barring an implementation of handshake.instance and a FIFO buffer.

Given the lack/low amount of users of Handshake, i'd be willing to sacrifice having support for lowering those operations right now, in favor of deprecating the FIRRTL pass. It'll unblock me in (as i see it) objectively improving the handshake dialect, and if anything it may serve as a motivation for other people to come in and add those lowerings.

... eventually, i'd also like to deprecate the handshake.memory-related things but let's leave that for another day :).

CC people that probably have an opinion here: @mikeurbach @Dinistro @RamirezLucas @darthscsi

Dinistro commented 1 year ago

That sounds like a great idea. Finally moving away from FIRRTL and also cleaning up handshake in the process will be beneficial in the long run. I still have to update my streaming work, but that should not be a hindering factor in this effort.

stephenneuendorffer commented 1 year ago

"Instead, i'd like to introduce a handshake type + a new operation handshake.unit which can wrap any other operation with unit-rate semantics (essentially the only "benefit" of the "every SSA value is a handshake" semantic)."

I'd like to hear more about your idea. From a semantic perspective, it's unclear to me how this would work if you're mixing handshake and non-handshake semantics in the same region. Not sure of what issues you've been running up against, but I infer from this that encapsulating non-handshake code in an operation has been cumbersome?

lucas-rami commented 1 year ago

Sounds great!

In the near future, I'll most-likely be doing a refactoring of the handshake dialect to remove the notion of "every SSA value is a handshaking connection". It was a neat idea, but in reality, it makes it impossible/really cumbersome to compose handshake and non-handshake code. Instead, i'd like to introduce a handshake type + a new operation handshake.unit which can wrap any other operation with unit-rate semantics (essentially the only "benefit" of the "every SSA value is a handshake" semantic).

I would be very interested in participating in any discussion about this and/or contributing to these changes. In the not so far future I will be looking into doing static-dynamic mixing in dataflow circuits, which is not currently possible (or rather it might only be possible to badly design it given the implicit semantics) at the handshake level, which I think is the abstraction level at which it makes the most sense to implement. I also initially liked the idea of implicit handshake semantics on every SSA value but I agree that it will be limiting in the long run. I was also never fond of having NoneType to represent dataless handshaked channels, but that's (sort of) orthogonal :)

... eventually, i'd also like to deprecate the handshake.memory-related things but let's leave that for another day :).

Also happy to be part of that discussion on this other day :)

mortbopet commented 1 year ago

@stephenneuendorffer

I'd like to hear more about your idea. From a semantic perspective, it's unclear to me how this would work if you're mixing handshake and non-handshake semantics in the same region. Not sure of what issues you've been running up against, but I infer from this that encapsulating non-handshake code in an operation has been cumbersome.

To me, the main issue is composability. The only way to compose the handshake dialect right now is to have handshake module instantiations + external handshake functions that eventually lower to a hw.module with a handshake-compatible interface. What if i want to have some parts of my module interface with a handshaking signal interface, and some others without? or what if i want to embed an FSM inside a module that uses handshake operators, but don't want that FSM to be a second class citizen wrt. control within the module (i.e. that FSM is forced to use handshake to communicate with other things). As you hint to, i think there should be a way to encapsulate non-handshake code in handshake, e.g:

%out = handshake.unit %in : !handshake<i32> -> (!handshake<i32>) {
^bb0(%in_data : i32):
  // join(inputs) on output is implicit.
   ... // -> return %out_data : i32
}

I haven't thought everything out in detail, however, i'd suspect my (future) proposal will be something where the standard way that handshake uses non-handshake operations is to embed them inside the above construct. That is, everything which uses a handshaking signaling standard does it through explicit types (e.g. handshake<T>) which (in hardware) lowers to ready/valid/data interfaces. Everything non-handshake can then live besides handshake and use whatever types, signaling standards, ... they'd like, without being shoehorned into handshake.

One counterpoint to this may be that you'd have a chain of non-handshake operations, like here which now would be a bunch of chained handshake.units with embedded logic. To me, that is a fair tradeoff. In practice, i think we've found that those kinds of handshake programs are never going to generate good hardware, so one should have, at IR construction time, deciding whether all of those chained operators should have gone into a single handshake.unit (which could also be a transformation pass, e.g. merging chained handshake.units), if it should have been a separate statically scheduled pipeline, ....

mikeurbach commented 1 year ago

Seems like there is an interesting discussion to be had around the handshake.unit proposal (and handshake.memory). Maybe we can separate that from the issue in the title of the ticket? Perhaps on its own issue, Discourse, or a CIRCT ODM, etc. since it seems there are interesting design decisions.

I vote to proceed with ripping out the FIRRTL lowering. Regardless of what happens with handshake, having one lowering path seems best, even if it may be missing some things today. HandshakeToFIRRTL had a good run, but I think we're all ready to focus on HandshakeToHW.

stephenneuendorffer commented 1 year ago

To me, the main issue is composability. Yes, this is really important :)

What if i want to have some parts of my module interface with a handshaking signal interface, and some others without? I think that there are 2, perhaps divergent, use models: 1) Modeling designs with KPN-type semantics, admitting transformations at this level. 2) Modeling RTL designs with handshake signals as a shorthand to avoid explicitly writing valid/ready signal everywhere.

It sounds like you're talking more about 2) than 1) here. The disadvantage to this approach is modifying such designs becomes very challenging... 2) definitely a useful intermediate lowering for composing systems with RTL components, but it's much harder to modify or optimize such designs. see, for example: https://ieeexplore.ieee.org/document/1173203

what if i want to embed an FSM inside a module that uses handshake operators Again, an FSM inside a dataflow design may have a different set of requirements than and FSM in a mostly RTL design.

I really like the handshake<T> for dealing with 2). If we're doing 1) and following strict hierarchical encapsulation, then every type is just wrapped with handshake and it seems like it becomes redundant? So, the fundamental question to me seems like whether 1) and 2) are different usages that can be effectively supported by some core concepts, or whether they are different (and perhaps inherently incompatible) and need to be supported by different dialects/types/etc.

I vote to proceed with ripping out the FIRRTL lowering. Regardless, I have to agree here :)

mortbopet commented 1 year ago

I'll start a discourse post wrt. the future of Handshake... it may even be as simple as, if one is using a handshake.func, handshake semantics are implicit, whereas if operations are nested in some other container operation, handshake semantics are explicit, via !handshake<T>-typed values.

mortbopet commented 1 year ago

Discourse post: https://discourse.llvm.org/t/should-ssa-values-in-handshake-always-have-implicit-handshake-semantics/70321

dtzSiFive commented 1 year ago

This looks to have been acted on in #5130, good to close?