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

[Arc] Remove obsolete arc.clock_tree and arc.passthrough ops #7704

Open
wants to merge 1 commit into
base: fschuiki/arc-new-lower-state
Choose a base branch
from

Conversation

fabianschuiki
Copy link
Contributor

The new LowerState pass does not produce arc.clock_tree and arc.passthrough ops anymore. Remove them from the dialect entirely.

@fabianschuiki fabianschuiki added the Arc Involving the `arc` dialect label Oct 14, 2024
Copy link
Contributor

@TaoBi22 TaoBi22 left a 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 😉)!

@fabianschuiki fabianschuiki force-pushed the fschuiki/arc-remove-clock-trees branch 2 times, most recently from 2c1bc29 to 7da872c Compare October 14, 2024 17:40
funcName.append("_passthrough");
builder.create<func::CallOp>(clockOp->getLoc(), funcName, TypeRange{},
ValueRange{funcOp.getBody().getArgument(0)});
}
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Contributor

@fzi-hielscher fzi-hielscher Oct 15, 2024

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@fzi-hielscher fzi-hielscher Oct 18, 2024

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.

@fabianschuiki fabianschuiki force-pushed the fschuiki/arc-remove-clock-trees branch 2 times, most recently from 2af2512 to b4d2ae4 Compare October 15, 2024 17:18
The new LowerState pass does not produce `arc.clock_tree` and
`arc.passthrough` ops anymore. Remove them from the dialect entirely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arc Involving the `arc` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants