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.

Sign up for GitHub

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

Jump to bottom

feat: add SMT-LIB overflow on addition for bitvectors BitVec.(uadd_overflow, sadd_overflow, uadd_overflow_eq, sadd_overflow_eq) and support theorems #6628

Merged
merged 67 commits into from
Feb 5, 2025
Merged

feat: add SMT-LIB overflow on addition for bitvectors BitVec.(uadd_overflow, sadd_overflow, uadd_overflow_eq, sadd_overflow_eq) and support theorems #6628

merged 67 commits into from
Feb 5, 2025

Conversation

luisacicolini
Copy link
Contributor

@luisacicolini luisacicolini commented Jan 13, 2025 •
edited
Loading

This PR adds SMT-LIB operators to detect overflow BitVec.(uadd_overflow, sadd_overflow), according to the definitions here, and the theorems proving equivalence of such definitions with the BitVec library functions (uaddOverflow_eq, saddOverflow_eq). Support theorems for these proofs are BitVec.toNat_mod_cancel_of_lt, BitVec.toInt_lt, BitVec.le_toInt, Int.bmod_neg_iff. The PR also includes a set of tests.

@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Jan 13, 2025
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Jan 13, 2025 •
edited
Loading

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 8483ac7258b84b179f3ec369df533452afdeb758 --onto d2c4471cfa4611977bf4927b5cd849df1a4272b7. (2025-01-13 14:04:32)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 8483ac7258b84b179f3ec369df533452afdeb758 --onto e9bd9807ef7a983365df9ac55d35040d0b2d5ef2. (2025-01-14 14:29:28)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 6ebce421423445eeac59a6f4347a562897a99e8d --onto f35a6020704e003995d585c7baa216982134d75f. (2025-01-24 08:01:44)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 6ebce421423445eeac59a6f4347a562897a99e8d --onto 69a73a18fbfa1fc045bfbf1c4cf93b155d4c9387. (2025-01-27 12:40:23)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 6ebce421423445eeac59a6f4347a562897a99e8d --onto 20c616503abe5ce4253c56dbcd7766a91c675ba0. (2025-01-28 11:12:24)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 6ebce421423445eeac59a6f4347a562897a99e8d --onto e922edfc218090ab2e54092336c67fe3b970dfc2. (2025-01-30 23:34:11)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 6ebce421423445eeac59a6f4347a562897a99e8d --onto ad48761032c5750e51289f3885967c1b6f8edde5. (2025-01-31 15:32:59)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 6ebce421423445eeac59a6f4347a562897a99e8d --onto 412389f71f8a24307e3adba69b38b4b685b04f72. (2025-02-04 14:00:37)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 8304bfe237a180ef4f3bf3147b36e1d0b67b502c --onto 412389f71f8a24307e3adba69b38b4b685b04f72. (2025-02-04 17:39:28)

@github-actions github-actions bot added the changelog-library Library label Jan 13, 2025
@luisacicolini
Copy link
Contributor Author

changelog-library

@luisacicolini luisacicolini marked this pull request as ready for review January 13, 2025 17:37
@luisacicolini luisacicolini requested a review from kim-em as a code owner January 13, 2025 17:37
@leanprover-bot leanprover-bot added the P-medium We may work on this issue if we find the time label Jan 14, 2025
@kim-em
Copy link
Collaborator

kim-em commented Jan 15, 2025

What are your plans for adding theorems about these?

@luisacicolini
Copy link
Contributor Author

Ideally supporting them in bvdecide - although there are some proofs missing (they're taking me more time than I expected), so I'll draft the PR for now and open it when the proofs are done!

@luisacicolini luisacicolini marked this pull request as draft January 15, 2025 08:34
@kim-em
Copy link
Collaborator

kim-em commented Jan 15, 2025

Okay! I'm happy with these definitions, and checked them against the reference, so I'm happy to click merge as soon as we're sure they are going to be used.

@luisacicolini luisacicolini changed the title feat: add SMT-LIB overflow definitions for bitvectors (not_overflow,uadd_overflow,sadd_overflow,umul_overflow,smul_overflow) feat: add SMT-LIB overflow on addition for bitvectors (uadd_overflow,sadd_overflow, uadd_overflow_eq,sadd_overflow_eq) Jan 22, 2025
luisacicolini and others added 2 commits January 22, 2025 12:51
@luisacicolini @tobiasgrosser
chore: formatting
3f33283 
Co-authored-by: Tobias Grosser <[email protected]>
@luisacicolini @tobiasgrosser
chore: formatting
95e3c01 
Co-authored-by: Tobias Grosser <[email protected]>
alexkeizer
alexkeizer reviewed Jan 22, 2025
View reviewed changes
src/Init/Data/BitVec/Basic.lean Outdated Show resolved Hide resolved
src/Init/Data/BitVec/Basic.lean Outdated Show resolved Hide resolved
src/Init/Data/BitVec/Lemmas.lean Outdated Show resolved Hide resolved
@luisacicolini @alexkeizer
chore: formatting
58de782 
Co-authored-by: Alex Keizer <[email protected]>
hargoniX
hargoniX reviewed Jan 30, 2025
View reviewed changes
src/Init/Data/BitVec/Lemmas.lean Outdated Show resolved Hide resolved
src/Init/Data/BitVec/Lemmas.lean Outdated Show resolved Hide resolved
alexkeizer
alexkeizer reviewed Jan 30, 2025
View reviewed changes
Copy link
Contributor

@alexkeizer alexkeizer left a comment

Choose a reason for hiding this comment

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

Some suggestions for more explicit docstring; feel to free to adapt/ignore

src/Init/Data/BitVec/Basic.lean Outdated Show resolved Hide resolved
src/Init/Data/BitVec/Basic.lean Outdated Show resolved Hide resolved
src/Init/Data/BitVec/Bitblast.lean Show resolved Hide resolved
bollu
bollu reviewed Feb 4, 2025
View reviewed changes
src/Init/Data/BitVec/Bitblast.lean Outdated
Comment on lines 1250 to 1260
have bmod_neg_iff {m : Nat} {x : Int} (h2 : -m ≤ x) (h1 : x < m) :
(x.bmod m) < 0 ↔ (-(m / 2) ≤ x ∧ x < 0) ∨ ((m + 1) / 2 ≤ x) := by
simp only [Int.bmod_def]
by_cases xpos : 0 ≤ x
· rw [Int.emod_eq_of_lt xpos (by omega)]; omega
· rw [Int.add_emod_self.symm, Int.emod_eq_of_lt (by omega) (by omega)]; omega
simp only [← decide_or, msb_eq_toInt, decide_beq_decide, toInt_add, ← decide_not, ← decide_and,
decide_eq_decide]
rw [bmod_neg_iff (by rw [← Nat.two_pow_pred_add_two_pow_pred (by omega)]; push_cast; omega)
(by rw [← Nat.two_pow_pred_add_two_pow_pred (by omega)]; push_cast; omega),
← @Nat.two_pow_pred_add_two_pow_pred (w + 1) (by omega), Nat.add_one_sub_one] at *
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish I could extract out a nice lemma out of this, but I'm sadly not sure what it should be. I suggest we get some more experience proving these theorems, and then consider a refactor of this part of the proof.

bollu
bollu approved these changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@bollu bollu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, but I wonder if @kim-em has a cleaner proof strategy for saddOverflow_eq. I'd be happy to clean up the proof in a later PR, once we have a bit more experience with these kinds of overflow proofs, so we can design a nice API.

@tobiasgrosser
Copy link
Contributor

awaiting-author

@github-actions github-actions bot added awaiting-author Waiting for PR author to address issues and removed awaiting-review Waiting for someone to review the PR labels Feb 4, 2025
@luisacicolini
Copy link
Contributor Author

lgtm!

bollu
bollu reviewed Feb 4, 2025
View reviewed changes
src/Init/Data/BitVec/Bitblast.lean Outdated
Comment on lines 1247 to 1252
have bmod_neg_iff {m : Nat} {x : Int} (h2 : -m ≤ x) (h1 : x < m) :
(x.bmod m) < 0 ↔ (-(m / 2) ≤ x ∧ x < 0) ∨ ((m + 1) / 2 ≤ x) := by
simp only [Int.bmod_def]
by_cases xpos : 0 ≤ x
· rw [Int.emod_eq_of_lt xpos (by omega)]; omega
· rw [Int.add_emod_self.symm, Int.emod_eq_of_lt (by omega) (by omega)]; omega
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to learn how to extract out a reusable lemma from this, but I'm happy to leave that as a task we perform once we get some more experience with the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

I moved this into Data/Int.

bollu
bollu reviewed Feb 4, 2025
View reviewed changes
Copy link
Contributor

@bollu bollu left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I register a place where I'd like to find an argument that goes through with a higher-level lemma.

@luisacicolini luisacicolini changed the title feat: add SMT-LIB overflow on addition for bitvectors (uadd_overflow,sadd_overflow, uadd_overflow_eq,sadd_overflow_eq) and support theorems feat: add SMT-LIB overflow on addition for bitvectors BitVec.(uadd_overflow, sadd_overflow, uadd_overflow_eq, sadd_overflow_eq) and support theorems Feb 5, 2025
@hargoniX hargoniX added this pull request to the merge queue Feb 5, 2025
Merged via the queue into leanprover:master with commit 0ed493e Feb 5, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobiasgrosser tobiasgrosser tobiasgrosser approved these changes

@bollu bollu bollu approved these changes

@alexkeizer alexkeizer alexkeizer left review comments

@hargoniX hargoniX hargoniX left review comments

@kim-em kim-em Awaiting requested review from kim-em kim-em is a code owner

Assignees
No one assigned
Labels
awaiting-author Waiting for PR author to address issues changelog-library Library P-medium We may work on this issue if we find the time toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@luisacicolini @leanprover-community-bot @kim-em @tobiasgrosser @bollu @alexkeizer @hargoniX @leanprover-bot