[FIRRTL] Don't enforce owning module for local ref in LowerClasses. #6811
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mikeurbach
requested review from
darthscsi,
dtzSiFive and
seldridge
as code owners
prithayan
approved these changes
Mar 11, 2024
We have an existing check that paths are in the same owning module as the entity they target. This is absolutely required for hierarchical references to compose with basepaths. However, we have legacy flows that extract entities out of their owning module, and we want to be able to target those things. This works around these conflicting requirements by skipping the owning module check for local targets only. This is safe because we still check such local targets are only instantiated once, so there is no ambiguity even though they are not instantiated within the owning module.
mikeurbach
force-pushed
the
mikeurbach/lower-classes-local-target
branch
from
080c0f1
to
acc9109
Compare
dtzSiFive
approved these changes
Mar 12, 2024
There was a problem hiding this comment.
Let's try to not let this live too long, but LGTM.
If filing an issue would help not forget then great but up to you.
Random thought: can the symbol become a hierpath due to dedup or something? If so, is that okay? Maybe it's fine anyway and not likely but just bouncing it off ya re: legality/safety.
Yeah I will file an issue to revert this once we stop using ExtractInstances
This can happen, and if it does the target will be subject to the existing "owning module" constraint. This only skips the constraint for targets that are still fully local by the time they reach LowerClasses. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have an existing check that paths are in the same owning module as the entity they target. This is absolutely required for hierarchical references to compose with basepaths. However, we have legacy flows that extract entities out of their owning module, and we want to be able to target those things. This works around these conflicting requirements by skipping the owning module check for local targets only. This is safe because we still check such local targets are only instantiated once, so there is no ambiguity even though they are not instantiated within the owning module.