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

[FIRRTL][Verif][LTL] Replace ltl.disable with an enable folded into verif.assert #7150

Merged
merged 36 commits into from
Jun 20, 2024

Conversation

dobios
Copy link
Member

@dobios dobios commented Jun 10, 2024

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 a uint1 to avoid errors like this in the future ^^"

Edit: This PR gets rid of the ltl.disable op and intrinsic in favor of having an enable operand on the verif assert like ops. This made it trivial to fix an incorrect when condition folding for AssertProperty. Most of the PR relates to fixing the chaos that was caused by removing ltl.disable.

@dobios dobios requested review from fabianschuiki and removed request for seldridge, darthscsi and dtzSiFive June 10, 2024 20:07
@dobios dobios changed the title [FIRRTL] Fixed when condition folding for Verif Intrinsics [FIRRTL] Fixed when condition folding for Verif Intrinsics Jun 10, 2024
@fabianschuiki
Copy link
Contributor

We should really have a better system than calling everything a uint1 to avoid errors like this in the future ^^"

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.

@dobios dobios requested a review from fabianschuiki June 11, 2024 01:28
@dobios
Copy link
Member Author

dobios commented Jun 11, 2024

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.

@dobios dobios marked this pull request as draft June 11, 2024 16:55
@dobios dobios changed the title [FIRRTL] Fixed when condition folding for Verif Intrinsics [FIRRTL][Verif][LTL] Replace ltl.disable with an enable folded into verif.assert Jun 11, 2024
Copy link
Member

@uenoku uenoku left a 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!

Copy link
Contributor

@fabianschuiki fabianschuiki left a 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!

Copy link
Contributor

@darthscsi darthscsi left a comment

Choose a reason for hiding this comment

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

Generally LGTM

Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

LGTM!

@dobios dobios merged commit ef30e1f into main Jun 20, 2024
4 checks passed
@dobios dobios deleted the dev/dobios/expandwhens branch June 20, 2024 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FIRRTL Involving the `firrtl` dialect verif
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants