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

[LTL] Add repeat and until operators #6989

Merged
merged 7 commits into from
May 18, 2024
Merged

Conversation

liuyic00
Copy link
Contributor

@liuyic00 liuyic00 commented May 5, 2024

Add repeat and until operators to LTL dialect.

This PR is separated from #6712.

Copy link
Member

@dobios dobios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for these additions, there are really useful !!
I left a few comments mostly about the wording of things (I want to avoid having ambiguous documentation for LTL because it is already subtle enough as is ^^"). Feel free to ping me when you've updated things!

docs/Dialects/LTL.md Outdated Show resolved Hide resolved
docs/Dialects/LTL.md Outdated Show resolved Hide resolved
docs/Dialects/LTL.md Outdated Show resolved Hide resolved
docs/Dialects/LTL.md Outdated Show resolved Hide resolved
docs/Dialects/LTL.md Outdated Show resolved Hide resolved

`ltl.until` is *weak*, meaning the property will hold even if the trace does not contain enough clock cycles to evaluate the property. `ltl.eventually` is *strong*, where `ltl.eventually %p` means `p` must hold at some point in the trace.

`always` is not implemented for now, but `always p` could be expressed by `not s_eventually not p`. Notice that assertions work globally. Properties will be checked from every cycle.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really tricky to encode when expressed this way (inverse liveness of an inverse property) using anything outside of actual SVA, so I'm a bit skeptical about including it here. Also the default is always when you don't specify anything, so it is in theory implemented, you just can't specify it explicitly because it is always implicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I mean the operator is not implemented. We can't write something like p |-> always q for now.
As for not s_eventually not p, maybe we can just leave it here until we have the plan to implement or encode always.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a question of semantic meaning, i.e. p |-> always q and always p |-> q are semantically equivalent, but p |-> eventually q and eventually p |-> q are not. This is one of the annoying subtleties of the always so in practice leaving it as the default will make it easier to encode. I'm just afraid that telling people to encode it as the negated liveness of a negated property will make it impossible to encode as anything other than a vendor-specific implementation of SVA. The end goal here is to have something that we can implement in open-source simulation, rather than relying on things that require weird heuristics to implement. This is just my opinion, but I think this particular sentence will lead to a lot of trouble in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a mistake I use p |-> because |-> in SVA only allows a sequence on the left side. However, we can consider the |-> as the logical implacation in this conversation.
p |-> always q means if p is true, then q needs to be always true for the reset of the time. But (always p) |-> q means if p is always true, then q needs be true. Or always (p |-> q) means every time p is true, then q needs to be true. They don't seem equivalent.

It's precisely because simulation that I don't think we need to worry about this liveness. In simulation, it dosen't actually check if the property eventually holds on the infinite trace, but only evaluates the property based on the existing simulation trace. Please let me know if I have a misunderstanding about what we want the simulation do with the properties.

include/circt/Dialect/LTL/LTLOps.td Outdated Show resolved Hide resolved
include/circt/Dialect/LTL/LTLOps.td Outdated Show resolved Hide resolved
Comment on lines 110 to 127
// CHECK: assert property (a[*0]);
// CHECK: assert property (a[*4]);
// CHECK: assert property (a[*5:6]);
// CHECK: assert property (a[*7:$]);
// CHECK: assert property (a[*]);
// CHECK: assert property (a[+]);
%r0 = ltl.repeat %a, 0, 0 : i1
%r1 = ltl.repeat %a, 4, 0 : i1
%r2 = ltl.repeat %a, 5, 1 : i1
%r3 = ltl.repeat %a, 7 : i1
%r4 = ltl.repeat %a, 0 : i1
%r5 = ltl.repeat %a, 1 : i1
verif.assert %r0 : !ltl.sequence
verif.assert %r1 : !ltl.sequence
verif.assert %r2 : !ltl.sequence
verif.assert %r3 : !ltl.sequence
verif.assert %r4 : !ltl.sequence
verif.assert %r5 : !ltl.sequence
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a lot clearer if you had one //CHECK per line, i.e.

// Check: assert property (a[*0]);
%r0 = ltl.repeat %a, 0, 0 : i1
verif.assert %r0 : !ltl.sequence

// CHECK: assert property (a[*4]);
....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is written in the same way as the code above, to keep all the tests about repeat in one code block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more of a question of not propagating bad habits

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they are both okay as long as they are consistent in the context. What does @fabianschuiki think about this? I could modify both here and previous codes.

test/Dialect/FIRRTL/lower-intrinsics.mlir Show resolved Hide resolved
@dobios dobios requested a review from fabianschuiki May 6, 2024 16:23
@liuyic00 liuyic00 requested a review from dobios May 10, 2024 14:15
Copy link
Member

@dobios dobios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Thank you so much for doing this, apart from a couple of minor comments (that I would greatly appreciate you take into account) LGTM!

Comment on lines 195 to 196
The operator of week `always` is not implemented for now, but week `always p` could be expressed by `not s_eventually not p`. Notice that assertions work globally. Properties will be checked from every cycle.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest removing this paragraph (see previous comment)

Comment on lines 110 to 127
// CHECK: assert property (a[*0]);
// CHECK: assert property (a[*4]);
// CHECK: assert property (a[*5:6]);
// CHECK: assert property (a[*7:$]);
// CHECK: assert property (a[*]);
// CHECK: assert property (a[+]);
%r0 = ltl.repeat %a, 0, 0 : i1
%r1 = ltl.repeat %a, 4, 0 : i1
%r2 = ltl.repeat %a, 5, 1 : i1
%r3 = ltl.repeat %a, 7 : i1
%r4 = ltl.repeat %a, 0 : i1
%r5 = ltl.repeat %a, 1 : i1
verif.assert %r0 : !ltl.sequence
verif.assert %r1 : !ltl.sequence
verif.assert %r2 : !ltl.sequence
verif.assert %r3 : !ltl.sequence
verif.assert %r4 : !ltl.sequence
verif.assert %r5 : !ltl.sequence
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try avoiding bad testing habits, even if for some reason they were used earlier, other tests in CIRCT usually do a line-by-line check

Copy link
Member

@SpriteOvO SpriteOvO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for your work! Waiting for CI to pass then we merge.

@SpriteOvO SpriteOvO merged commit d40f528 into llvm:main May 18, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants