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

[HandshakeToDC] Add pack/unpack lowering patterns #6941

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

mortbopet
Copy link
Contributor

@mortbopet mortbopet commented Apr 22, 2024

Adds lowering patterns for handshake.pack/unpack to DC. For the actual (data-side) tuple operations, hw.structs are used. This is purely for pragmatic reasons; Firstly, hw.struct is for all intents and purposes a generic struct type. Secondly, we don't want data-only operations to leak into DC.

@teqdruid
Copy link
Contributor

This is entirely for lowering handshake.func args to a tuple? If so, why not use a struct with field names matching the arg names?

@mortbopet
Copy link
Contributor Author

@teqdruid what struct type? hw.struct? If so, you know my opinion about that :)

@teqdruid
Copy link
Contributor

hw.struct is the struct type in CIRCT and both Handshake and DC are in CIRCT. It's the practical choice. And it's not tied to hardware in any particular semantic -- just the name. We wouldn't have it there if there was a struct type upstream we could use!

I think keeping the argument names around is more important than dialect name purity. And I know you share my aversion to duplicating stuff in each dialect.

Plus, there's an argument to be made that there's nothing in the hw or comb dialects that are specifically tied to hardware.

@mortbopet
Copy link
Contributor Author

Plus, there's an argument to be made that there's nothing in the hw or comb dialects that are specifically tied to hardware.

... except for - for the former - the dialect name itself 🙂.

You have a good point, hw.struct is - if we ignore the dialect prefix - an overall more well suited type for this than tuple.

@Dinistro
Copy link
Contributor

Side note: There was some discussion at EuroLLVM about introducing a general dialect to model struct types in MLIR. There are multiple places where this would be helpful, i.e., Flang, LLVM dialect, CIR, and CIRCT.
I'm not sure what the status of this is, but it might make sense to try and raise this properly on discourse.

@mortbopet mortbopet changed the title [DC] Add data-only pack/unpack operations [HandshakeToDC] Add pack/unpack lowering patterns Apr 26, 2024
@mortbopet
Copy link
Contributor Author

@teqdruid updated the PR s.t. this is now strictly a HandshakeToDC PR, which uses hw.struct.

@mortbopet mortbopet force-pushed the dev/mpetersen/dc_packunpack branch 2 times, most recently from b5d5325 to bebaf6f Compare April 26, 2024 12:03
@mortbopet mortbopet force-pushed the dev/mpetersen/dc_packunpack branch 2 times, most recently from 1125284 to 99f5769 Compare May 7, 2024 08:45
mortbopet and others added 2 commits November 13, 2024 22:32
Adds data-only pack/unpack operations which operates on MLIR tuple-typed values. Don't want these in DC, but there really isn't any other place to put them; and they are required for an end-to-end handshake lowering path.

Add tuple packing dc-to-hw lowering

remove DC stuff, add tuple -> Hw conversion in Handshake to DC
@teqdruid teqdruid merged commit 0b5c1a8 into main Nov 13, 2024
4 checks passed
@teqdruid teqdruid deleted the dev/mpetersen/dc_packunpack branch November 13, 2024 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants