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] Support alternative base paths in LowerClasses. #6817

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

mikeurbach
Copy link
Contributor

We previously had a constraint that paths are targeting entities in the same owning module as the path. This allowed us to assume that a single base path can be created for each owning module, and used in all paths. However, we have transforms that extract entities targeted by paths outside the paths' owning module.

To support this, we can instead plumb through base paths created higher up in the instance graph down to the paths that need them. When a path is detected outside the owning module, we find the module in which the entity is instantiated and pass through its base path to the paths that reference the entity.

Most of the work here is just plumbing, and the PathInfoTable has been extended with new data structures and functions to support this.

@mikeurbach
Copy link
Contributor Author

Hmm seems like some minor issue that I didn't see while working on this with an older LLVM version, I'll fix that.

@mikeurbach mikeurbach force-pushed the mikeurbach/lower-classes-alt-base-path branch from 7bc4de5 to 27269f0 Compare March 13, 2024 15:24
@mikeurbach
Copy link
Contributor Author

I don't know what's going on... I didn't see these errors locally 🤦

We previously had a constraint that paths are targeting entities in
the same owning module as the path. This allowed us to assume that a
single base path can be created for each owning module, and used in
all paths. However, we have transforms that extract entities targeted
by paths outside the paths' owning module.

To support this, we can instead plumb through base paths created
higher up in the instance graph down to the paths that need them. When
a path is detected outside the owning module, we find the module in
which the entity is instantiated and pass through its base path to the
paths that reference the entity.

Most of the work here is just plumbing, and the PathInfoTable has been
extended with new data structures and functions to support this.
@mikeurbach mikeurbach force-pushed the mikeurbach/lower-classes-alt-base-path branch from 27269f0 to 3bd4a6d Compare March 13, 2024 17:33
Co-authored-by: Robert Young <rwy0717@gmail.com>
Copy link
Contributor

@prithayan prithayan left a comment

Choose a reason for hiding this comment

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

Looks good.

lib/Dialect/FIRRTL/Transforms/LowerClasses.cpp Outdated Show resolved Hide resolved
Co-authored-by: Prithayan Barua <prithayan@gmail.com>
@mikeurbach mikeurbach merged commit fed17ee into main Mar 13, 2024
4 checks passed
@mikeurbach mikeurbach deleted the mikeurbach/lower-classes-alt-base-path branch March 13, 2024 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants