-
Notifications
You must be signed in to change notification settings - Fork 296
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
[Arc] Remove obsolete arc.clock_tree and arc.passthrough ops #7704
Open
fabianschuiki
wants to merge
1
commit into
fschuiki/arc-new-lower-state
Choose a base branch
from
fschuiki/arc-remove-clock-trees
base: fschuiki/arc-new-lower-state
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
Oops, something went wrong.
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.
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.
If the initial does affect an output port value directly, do we not need to guarantee that this change is propagated to the output port?
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.
Yeah that's a good question. Since passthrough is now gone, this boils down to whether the initial function should also call the eval function to propagate the effects of the initial assignments to outputs and side-effecting ops. I'm trying to think whether there is a use case where the user wants to run
initial
, but then do some other tweaking (or maybe dump a first timestep to VCD?) before calling the firsteval
and potentially triggering output or state changes.@uenoku Has observed that some simulators have fairly intricate bring-up sequences. Generally it seems like there's an initial value assignment to variables, often X, then an execution of all variable initializers, then executing all
initial
blocks, and then triggering anyposedge
and the like based on those variables. We might need to do something similar at some point? Maybe some form of tieredarc.initial
to have containers for the different rounds of initialization?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.
That is what I've been desperately trying to avoid so far. But as the SV LRM so nonchalantly states on initialization at declaration:
Considering that commercial simulators also tend to take their freedoms here [citation needed], I was hoping we could avoid that madness. I feel when we have to debate whether a
posedge
has occurred when abit
is default initialized to 0, then initialized to 1 at declaration, and then initialized back to 0 in aninitial
process, we have already lost.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.
Yeah I totally agree. We should probably imitate the basic fixing of Verilog semantics that simulators do with their observable X-to-initial-value transitions, because otherwise Verilog's
always @(...)
combinational processes and async resets are virtually unusable. But we should do as little as possible/necessary.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.
For the triggered processes over in #7676 I currently require a tieoff constant for all processes, which also serves as pre-initial value. I've thought about changing this to take
!seq.immutable
values. Considering that neither me nor @uenoku have intended it that way, I think this would mesh surprisingly well. It would de facto pushseq.initial
into the pre-initial phase and we wouldn't be able to use non-!seq.immutable
values in that phase. But I guess this is what we already arrived at in #7638 ?!?But to go back to Martin's original question here: I think it is a fair choice to not force propagation after initialization. In the current implementation I've ended up introducing an inconsistency between models without an initializer (which won't call
passthrough
at startup) vs. the same model with an empty initializer (which will callpassthrough
). It's a good thing to avoid that.