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

[Handshake] Adding func instance op for integration #7812

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

teqdruid
Copy link
Contributor

@teqdruid teqdruid commented Nov 13, 2024

Adds the ESIInstanceOp. This op allows a non-handshake design to instantiate handshake.funcs. Since handshake needs elastic inputs and produces elastic outputs, this uses ESI channels.

@teqdruid
Copy link
Contributor Author

@mortbopet This is the op we discussed earlier today. Posting it without the lowering to get your eyes on it before you go on vacation. Will add the lowering before merging.

@mortbopet
Copy link
Contributor

Some nit comments, but yep, this LGTM. I'm assuming this will lower to a hw.instance + dc.from_esi/dc.to_esi in HandshakeToDC.

@teqdruid
Copy link
Contributor Author

I'm assuming this will lower to a hw.instance + dc.from_esi/dc.to_esi in HandshakeToDC.

Yep. Thanks!

@teqdruid
Copy link
Contributor Author

I'm assuming this will lower to a hw.instance + dc.from_esi/dc.to_esi in HandshakeToDC.

Yep. Thanks!

Actually, the DC lowering appears to be broken (#7818). I've added it instead to the HandshakeToHW lowering. Will add it to the DC on once that's fixed.

Adds the ESIInstanceOp. This op allows a non-handshake design to
instantiate `handshake.func`s. Since handshake needs elastic inputs and
produces elastic outputs, this uses ESI channels.
@teqdruid teqdruid marked this pull request as ready for review November 14, 2024 21:55
@teqdruid
Copy link
Contributor Author

Will rely on post-merge feedback since @mortbopet is on vacation.

@teqdruid teqdruid merged commit a40aed0 into main Nov 15, 2024
4 checks passed
@teqdruid teqdruid deleted the teqdruid/handshake/esicall branch November 15, 2024 20:00
Copy link
Contributor

@Dinistro Dinistro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not checking this earlier. I'm no longer deeply involved in handshake and my current job is quite consuming at the moment.

Non-the-less, this change looks reasonable to me.

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