-
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
base: fschuiki/arc-new-lower-state
Are you sure you want to change the base?
[Arc] Remove obsolete arc.clock_tree and arc.passthrough ops #7704
Conversation
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 (once it builds 😉)!
252420f
to
e648462
Compare
2c1bc29
to
7da872c
Compare
e648462
to
821d12f
Compare
funcName.append("_passthrough"); | ||
builder.create<func::CallOp>(clockOp->getLoc(), funcName, TypeRange{}, | ||
ValueRange{funcOp.getBody().getArgument(0)}); | ||
} |
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 first eval
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 any posedge
and the like based on those variables. We might need to do something similar at some point? Maybe some form of tiered arc.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:
This may require a special pre-initial pass at run time.
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 a bit
is default initialized to 0, then initialized to 1 at declaration, and then initialized back to 0 in an initial
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 push seq.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 call passthrough
). It's a good thing to avoid that.
821d12f
to
2b87d2e
Compare
2af2512
to
b4d2ae4
Compare
2b87d2e
to
8c64305
Compare
b4d2ae4
to
8bef704
Compare
8c64305
to
ca2369f
Compare
The new LowerState pass does not produce `arc.clock_tree` and `arc.passthrough` ops anymore. Remove them from the dialect entirely.
8bef704
to
fce7727
Compare
ca2369f
to
401c3cf
Compare
The new LowerState pass does not produce
arc.clock_tree
andarc.passthrough
ops anymore. Remove them from the dialect entirely.