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] enable properties for inherent attributes #7750

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

youngar
Copy link
Member

@youngar youngar commented Oct 30, 2024

[FIRRTL] Clean up inferReturnTypes implementation
739cf51

This is a big change to clean up our implementation of
inferReturnTypes in an effort to get ourselves ready to enable
properties in the FIRRTL dialect. We had structured our API so that
inferReturnType would call out to a simpler version of itself with
some of the useless arguments removed. This prevented us from using op
adaptors to abstract over whether or not an inherent attribute was
contained inside the attr-dict or the properties.

This change keeps the old structure of a two level API, but with
different boundaries: The large API takes all arguments, creates an
adapter, pulls out the necessary attributes and then calls in to the
simpler interface. FIRParser uses the simpler API when inferring return
types. The simpler interface is now specific to each operation and not
common with other operations.

This last change caused a problem for parsePrimExp, which relied on a
generic interface to create all expressions. This function is now
templated over exactly how many arguments the specific prim op takes,
parses exactly that many, and splats them out when calling
inferReturnType. As an upside to this, we can also call a more
specific builder for each operation, which should speed up building
operations when we move to properties.


[FIRRTL] enable properties for inherent attributes
f38c9b2
This enables properties in the FIRRTL dialect. Some op parsers which
called inferReturnTypes had to be updated to store inherent attributes
as properties on the OperationState, as the call to inferReturnTypes
expects properties to have already been populated. This is a change
that we should make for all operation parsers and builders, to make
things faster, but is not strictly necessary at this time.

@youngar youngar added the FIRRTL Involving the `firrtl` dialect label Oct 30, 2024
@youngar youngar force-pushed the firrtl-properties branch 3 times, most recently from f38c9b2 to 5dbb930 Compare October 30, 2024 03:08
@youngar youngar marked this pull request as ready for review October 30, 2024 03:22
@youngar
Copy link
Member Author

youngar commented Oct 30, 2024

On a firtool run, the change in performance is pretty close to a wash. Most passes got a little slower, some had bigger changes. This is newtime/oldtime again:
PrefixModules: %128
DropName: %36
Will do some profiling to see what is so slow.

This is a big change to clean up our implementation of
`inferReturnTypes` in an effort to get ourselves ready to enable
properties in the FIRRTL dialect. We had structured our API so that
`inferReturnType` would call out to a simpler version of itself with
some of the useless arguments removed.  This prevented us from using op
adaptors to abstract over whether or not an inherent attribute was
contained inside the attr-dict or the properties.

This change keeps the old structure of a two level API, but with
different boundaries: The large API takes all arguments, creates an
adapter, pulls out the necessary attributes and then calls in to the
simpler interface. FIRParser uses the simpler API when inferring return
types.  The simpler interface is now specific to each operation and not
common with other operations.

This last change caused a problem for `parsePrimExp`, which relied on a
generic interface to create all expressions. This function is now
templated over exactly how many arguments the specific prim op takes,
parses exactly that many, and splats them out when calling
`inferReturnType`.  As an upside to this, we can also call a more
specific builder for each operation, which should speed up building
operations when we move to properties.
This enables properties in the FIRRTL dialect. Some op parsers which
called `inferReturnTypes` had to be updated to store inherent attributes
as properties on the OperationState, as the call to `inferReturnTypes`
expects properties to have already been populated.  This is a change
that we should make for all operation parsers and builders, to make
things faster, but is not strictly necessary at this time.
Copy link
Contributor

@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

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

Really cool to see this migrated over to properties. LGTM!

@youngar youngar merged commit f22f11a into llvm:main Oct 30, 2024
4 checks passed
@youngar youngar deleted the firrtl-properties branch October 30, 2024 22:02
@youngar
Copy link
Member Author

youngar commented Oct 30, 2024

Thanks for taking a look!

@seldridge
Copy link
Member

Impressive about DropNames. That has been such a trivial pass that has resisted speedups. A 2.7x speedup on that is awesome.

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.

4 participants