-
Notifications
You must be signed in to change notification settings - Fork 305
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
[LTL] Add clocked and disabled property types #7044
Conversation
lib/Dialect/LTL/LTLTypes.cpp
Outdated
return isa<ClockedPropertyType>(type) || isa<ClockedSequenceType>(type) || | ||
isa<ClockedDisabledPropertyType>(type); |
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.
FYI you can pass multiple classes to isa<>
, e.g:
return isa<ClockedPropertyType>(type) || isa<ClockedSequenceType>(type) || | |
isa<ClockedDisabledPropertyType>(type); | |
return isa<ClockedPropertyType, ClockedSequenceType, ClockedDisabledPropertyType>(type); |
From an implementation point of view this looks great! I'm wondering if it's worth the complication given what #7022 originally tried to achieve. If I recall correctly, we basically want a
The one thing that strikes me as odd about the encoding in the type system is that it feels like it's answering a question that isn't useful outside of
So if any operand is a clocked property, the result is also a clocked property. This makes the type answer the question of whether there is any nested property/sequence that is clocked. This feels somewhat unintuitive, since the %0 = ltl.delay %a : sequence
%1 = ltl.clock %0 : clocked_sequence
%2 = ltl.delay %b : sequence
%3 = ltl.and %1, %2 : clocked_sequence The fact that What are your thoughts on doing the verification in a dedicated pass? That sounds like it might be simpler than extending the LTL type system. |
There is a 4th way of fixing this issue: Remove the |
Hey @ekiwi! Thanks for the comment! The one reason we would want to keep assert property (@(posedge clk) disable iff (cond) (a |-> (@(posedge fclk) b ##1 c))) and we would not be able to express that without having That being said, AFAIK we can't actually express that in Chisel (correct me if I'm wrong) but so far the only way I was able to find to connect a clock to a property was via |
I never considered this case before, thanks for pointing it out. Do you know what the semantics of this are? Is the property essentially active on either clock edge and then certain automaton transitions are only available depending on which event triggered it? Anyways, I do feel like once you are in a position where this feature is required, you might be able to come up with a better solution than the very generic |
SV basically has a clock resolution process, where it will check the sequence using the closest parenting clock. So in the case of the example I showed, the consequent sequence in the overlapping implication is describing |
After further reasoning, I don't think that this is a good approach, so I'm closing the PR. |
#7022 highlighted a difficulty in reasoning about certain characteristics of ltl properties, e.g. finding out how many clocks or disables are associated to a given property without resorting to a use-def analysis.
This PR attempts to solve that by encoded those concepts directly in ltl's type-system.
Let me know what you think!