-
Notifications
You must be signed in to change notification settings - Fork 313
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] FART: allow modules to be in multiple domains at once #8172
Conversation
I think this fixes #7611? |
Yeah, this fixes that issue. This also fixes issue #7271 to error if the pre-existing reset port has the wrong type. (will update the commit message to reflect this) |
ff5e7d9
to
ea07788
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Have you tested this out internally? I'm curious if this change to require unique reset names is going to cause a breakage during the next CIRCT bump. 🧐
/// Whether this is the root of the reset domain. | ||
bool isTop = false; | ||
|
||
/// The reset signal for this domain. A null value indicates that this domain | ||
/// explicitly has no reset. | ||
Value reset; | ||
|
||
// Implementation details for this domain. | ||
Value existingValue; | ||
Value rootReset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the current interaction between isTop
and rootReset
? Is the former now unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, rootReset
is really only there for error messages. I do think there is some redundant information in this struct, but I am not sure how to factor it. In the end I decided to just keep it simple, but maybe we can tackle this more in the future? For example, I am not sure why buildDomains
and determineImpl
are separate phases. If we squash them in to one, we don't have to hold on to so much information, and could delete the isTop
field. As well, something like the existingPort
could be determined by casting the local reset to a block arg and asking for the port number. There are probably some other things we could do.
When a child module is underneath two different reset domains, the pass would emit an error message. This change makes it so that it is allowed as long as the reset signal name and the reset signal type between the two domains are the same. This also changes the logic to re-use a preexisting reset port: if there is a port with the correct name for a reset domain, but the type does match, we used to pick a different port name. This changes it to be an error, which matches the old SFC behaviour.
When a child module is underneath two different reset domains, the pass
would emit an error message. This change makes it so that it is allowed
as long as the reset signal name and the reset signal type between the
two domains are the same.
This also changes the logic to re-use a preexisting reset port: if there
is a port with the correct name for a reset domain, but the type does
match, we used to pick a different port name. This changes it to be an
error, which matches the old SFC behaviour.