-
Notifications
You must be signed in to change notification settings - Fork 175
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
[RFC] Clarify the meaning of Past and friends when there is no lexically enclosing scope #539
Comments
Actually, Upside n. 1 above should read:
|
Personally, I agree with whitequark@'s mental model that x = Signal(8)
m.d.clk1 += x.eq(x+1) So we can have: m.d.comb += Assert((Past(x, domain="clk1") + 1)[:8] == x) That is, no matter what formal verification step I'm on, and no matter how that relates to The proposed syntax would be: m.d.clk1 += Assert((Past(x) + 1)[:8] == x) To me, this has similar syntax to: m.d.clk1 += x.eq(x+1) Now, I can always refactor: next_x = Signal(8)
m.d.comb += next_x.eq(x+1)
m.d.clk1 += x.eq(next_x) Now consider again the proposed syntax for m.d.clk1 += Assert((Past(x) + 1)[:8] == x) In a similar way, I can also refactor: compare_x = Signal(8)
m.d.comb += compare_x.eq(Past(x) + 1)
m.d.clk1 += Assert(compare_x == x) But this is now wrong, because the syntax proposes that |
I concur. To add to (or perhaps rephrase) what @RobertBaruch said, I think that it is important that any subexpression can be replaced with either (a) Python variable containing that subexpression, or (b) fresh |
I propose this RFC be closed, since it has fatal issues, or the original proposer should modify the proposal to eliminate the problems. |
Sure. Thanks for the insights and feedback. I admit I don't really have a use case for multi-clock formal verification, and will respectfully defer to the opinion of those that do. |
I would like to propose an alternative to the solution being discussed on #526. In order to not derail that discussion, and since it's somewhat opposite to it, I opened this RFC.
To recapitulate, in #526 (comment), @whitequark said:
In #526 (comment), I pointed out:
The model being discussed in #526 is:
Also,
This RFC takes the point of view that the current model can be fixed, with some clarifications.
Basically, it's:
with m.If(Past(..., domain=<domain>)):
2.1. In case the domain is not specified, assume sync.
domain="comb"
.As for "magically lifting the domain from the lexically containing statement", I would argue that it is consistent with the way assignments already work in nMigen.
Upsides:
m.d.<domain> += Assert(Past(...))
instead ofm.d.<domain> += Assert(Past(..., domain=<domain>))
m.d.<domain> += [ a.eq(b), Assert(Past(c)) ]
m.d.<domain1> += Assert(Past(...,domain=<domain2>))
Downsides:
The text was updated successfully, but these errors were encountered: