-
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
[FIRRTL][Verif][LTL] Replace ltl.disable
with an enable folded into verif.assert
#7150
Conversation
when
condition folding for Verif Intrinsics
I totally agree 😀. @uenoku had some thoughts a while back on adding intrinsic types that served a similar role as intrinsic ops. Types are a lot trickier to get right though -- I remember there were a lot of concerns about a generic opaque intrinsic type. Although it would solve this particular problem here. |
So after some discussion, this PR will shift focus towards replacing disable intrinsics with an enable that is folded into the verif intrinsics. This will make the fix in expandWhens as simple as for the other assertion type. |
when
condition folding for Verif Intrinsicsltl.disable
with an enable folded into verif.assert
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.
Generally looks great! Thank you for cleaning up!
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.
It's great to see ltl.disable
go 🥳 Thanks for going through all this effort!
The logic of the changes looks good to me!
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.
Generally LGTM
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!
This PR fixes a bug in ExpandWhens that incorrectly folded the when condition into the property.We should really have a better system than calling everything auint1
to avoid errors like this in the future ^^"Edit: This PR gets rid of the
ltl.disable
op and intrinsic in favor of having anenable
operand on the verif assert like ops. This made it trivial to fix an incorrectwhen
condition folding forAssertProperty
. Most of the PR relates to fixing the chaos that was caused by removingltl.disable
.