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

Apply simplification to Imp and Iff connectives #4533

Merged
merged 37 commits into from
Apr 2, 2024
Merged

Conversation

zafer-esen
Copy link
Collaborator

@zafer-esen zafer-esen commented Sep 11, 2023

Boogie applies simplification to Bpl.And and Bpl.Or, but not to Bpl.Imp and Bpl.Iff. Dafny provides wrapper methods to apply these connectives which also apply simplification to Imp and Iff; however they do not seem to be used consistently and are missing some cases.

This PR replaces all direct usages of the Boogie methods Bpl.And, Bpl.Or, Bpl.Imp and Bpl.Iff with wrappers from Dafny, and adds code to avoid adding true axioms in cases where I noticed this happens. (A better solution might be to provide a single point for adding axioms where this check could be added.)

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

@zafer-esen zafer-esen marked this pull request as draft September 11, 2023 14:05
@atomb atomb self-assigned this Jan 23, 2024
@atomb atomb marked this pull request as ready for review January 31, 2024 22:09
@@ -177,7 +177,7 @@ method EuclidGcd(X: pos, Y: pos) returns (gcd: pos)
// ------------------------------------------------------------------------------------------------------
// The alternative definitions that follow allow the two cases in the GCD algorithm to look more similar.

lemma GcdSubtractAlt(x: pos, y: pos)
lemma {:resource_limit "150e6"} GcdSubtractAlt(x: pos, y: pos)
Copy link
Member

@keyboardDrummer keyboardDrummer Feb 1, 2024

Choose a reason for hiding this comment

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

Why is this now needed? If we're improving verification performance, why would we need to raise resource limits?

Copy link
Member

Choose a reason for hiding this comment

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

That example was just incredibly brittle, and for things in that state it seems that any change, even one that overall should be a simplification, can throw things off the rails. Changing things as simple as the filenames passed in as arguments to the :captureState attribute was enough to change resource use by an order of magnitude. I've refactored that test, and one other, to be less brittle, and they no longer require raised resource limits.

The remaining failures will, I believe, be fixed by brittleness reductions included in #4597, so let's merge that one first, update this PR, and see if everything is good.

keyboardDrummer
keyboardDrummer previously approved these changes Feb 1, 2024
atomb added 2 commits February 1, 2024 09:14
This one exhibited order-of-magnitude changes in resource use due to
changes as simple as the length of the filename in each :captureState
attribute. Making Gcd opaque reduced resource use by at least two
orders of magnitude.
@@ -303,7 +303,7 @@ abstract module {:disableNonlinearArithmetic} Std.Arithmetic.LittleEndianNat {

/* The nat representation of a sequence and its least significant position are
congruent. */
lemma LemmaSeqLswModEquivalence(xs: seq<digit>)
lemma {:resource_limit "10e6"} LemmaSeqLswModEquivalence(xs: seq<digit>)
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, it looks like it isn't! Good catch.

keyboardDrummer
keyboardDrummer previously approved these changes Apr 2, 2024
@atomb atomb enabled auto-merge (squash) April 2, 2024 15:54
keyboardDrummer
keyboardDrummer previously approved these changes Apr 2, 2024
@atomb atomb merged commit 2cc052c into master Apr 2, 2024
20 checks passed
@atomb atomb deleted the simplify-imp-iff branch April 2, 2024 18:50
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.

3 participants