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

[FIRRTL] infer more return types in asm format #7781

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

Conversation

youngar
Copy link
Member

@youngar youngar commented Nov 7, 2024

Many expressions in FIRRTL implement the InferTypesOpInterface, but we include an explicit result type in the assembly format. This change removes the explicit result type in the asm format for many FIRRTL expressions, which means less typing.

Many expressions in FIRRTL implement the InferTypesOpInterface, but we
include an explicit result type in the assembly format.  This change
removes the explicit result type in the asm format for many FIRRTL
expressions, which means less typing.
@youngar youngar added the FIRRTL Involving the `firrtl` dialect label Nov 7, 2024
@seldridge
Copy link
Member

I think my largest gripe about our FIRRTL Dialect syntax is that it is inconsistent in where this kind of choice is applied. E.g., some operations are very explicit with (a, b) -> (c) and others are more abbreviated like firrtl.probe and firrtl.matchingconnect. I think consistency here is a good thing. This pulls it more in the direction of abbreviation which is most prevalent in newer operations.

This seems reasonable.

@darthscsi: WDYT?

@darthscsi
Copy link
Contributor

Yea, function type style printing was generally from early circt time. I don't think all the type elision options existed when some of these ops were added. But the intention was always to clean this up.

@darthscsi
Copy link
Contributor

Although this is a structuralish change and I have no problem with it going in a sweeping change, it might be more manageable to do it op at a time (just to get it unstuck).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FIRRTL Involving the `firrtl` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants