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

feat: BitVec.{toInt, toFin, msb}_udiv #6402

Merged
merged 4 commits into from
Jan 10, 2025
Merged

Conversation

alexkeizer
Copy link
Contributor

@alexkeizer alexkeizer commented Dec 16, 2024

This PR adds a toFin and msb lemma for unsigned bitvector division. We don't have toInt_udiv, since the only truly general statement we can make does no better than unfolding the definition, and it's not uncontroversially clear how to unfold toInt (see toInt_eq_msb_cond/toInt_eq_toNat_cond/toInt_eq_toNat_bmod for a few options currently provided). Instead, we do have toInt_udiv_of_msb that's able to provide a more meaningful rewrite given an extra side-condition (that x.msb = false).

This PR also upstreams a minor Nat theorem (Nat.div_le_div_left) needed for the above from Mathlib.

@alexkeizer alexkeizer requested a review from kim-em as a code owner December 16, 2024 14:39
@alexkeizer alexkeizer marked this pull request as draft December 16, 2024 14:49
Comment on lines +2586 to +2646
theorem msb_udiv (x y : BitVec w) :
(x / y).msb = (x.msb && y == 1#w) := by
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
theorem msb_udiv (x y : BitVec w) :
(x / y).msb = (x.msb && y == 1#w) := by
theorem msb_udiv {x y : BitVec w} :
(x / y).msb = (x.msb && y == 1#w) := by

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'm curious what rule people use to determine whether variable should be explicit or implicit; I think we do this pretty inconsistently in the file at the moment.

For me, I try to make variables implicit only if they can be inferred from the other hypotheses, not including the lhs of an equality or bi-implication, but make sure that any explicit variables that can be inferred from the lhs come at the end of the argument list. Hence, x and y here are explicit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree it is ambiguous, and inconsistent.

Note, however, that at least for a bi-implication the standard (at least, as enforced by a linter in Batteries) is that variables appearing on both sides of a bi-implication should be implicit.

I tend to follow (erratically!) the same rule for equalities, too.

Let's not get hung up on this for now.

@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 Dec 16, 2024
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Dec 16, 2024

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 1b15a0f27cfdedfc81470b1720f1d603417192e7 --onto a8a160b09147c3225150703ac727eea6ee9a3b0e. (2024-12-16 15:24:36)
  • 💥 Mathlib branch lean-pr-testing-6402 build failed against this PR. (2025-01-09 15:41:45) View Log
  • 💥 Mathlib branch lean-pr-testing-6402 build failed against this PR. (2025-01-10 02:39:20) View Log

@alexkeizer
Copy link
Contributor Author

Mathlib CI status (docs):

* ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the `nightly-with-mathlib` branch. Try `git rebase 1b15a0f27cfdedfc81470b1720f1d603417192e7 --onto a8a160b09147c3225150703ac727eea6ee9a3b0e`. (2024-12-16 15:24:36)

I tried to rebase on the suggested hash, but this fails because I've modified a file that didn't yet exist in that version. I could resolve the conflict, but that seems like it might cause annoying merge conflicts later down the line.

Regardless, I've upstreamed a Nat lemma (Nat.div_le_div_left), so Mathlib will break, but I've upstreamed the statement as-is, so the fix should be as easy as removing the Mathlib version of the lemma

@alexkeizer
Copy link
Contributor Author

This should now be ready for review, but since I'm on holidays I'll keep it as a draft until I'm back and can respond to reviews timely. Hopefully by then nightly-with-mathlib will have caught up, and I can rebase on it without trouble.

@alexkeizer alexkeizer marked this pull request as ready for review January 9, 2025 14:51
@alexkeizer
Copy link
Contributor Author

awaiting-review

I just rebased on nightly-with-mathlib also

@github-actions github-actions bot added the awaiting-review Waiting for someone to review the PR label Jan 9, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jan 9, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 9, 2025
@leanprover-community-bot leanprover-community-bot added the breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan label Jan 9, 2025
@alexkeizer
Copy link
Contributor Author

changelog-library

@github-actions github-actions bot added the changelog-library Library label Jan 9, 2025
@kim-em
Copy link
Collaborator

kim-em commented Jan 10, 2025

Hm, that's said, the CI step checking labels didn't get triggered automatically. :-(

@kim-em kim-em added changelog-library Library and removed changelog-library Library labels Jan 10, 2025
@kim-em kim-em enabled auto-merge January 10, 2025 02:13
@kim-em kim-em removed the awaiting-review Waiting for someone to review the PR label Jan 10, 2025
@kim-em kim-em added this pull request to the merge queue Jan 10, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Jan 10, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Jan 10, 2025
Merged via the queue into leanprover:master with commit d2c4471 Jan 10, 2025
16 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Jan 10, 2025
This PR adds a `toFin` and `msb` lemma for unsigned bitvector modulus.
Similar to #6402, we don't provide a general `toInt_umod` lemmas, but
instead choose to provide more specialized rewrites, with extra
side-conditions.

---------

Co-authored-by: Kim Morrison <[email protected]>
luisacicolini pushed a commit to opencompl/lean4 that referenced this pull request Jan 21, 2025
This PR adds a `toFin` and `msb` lemma for unsigned bitvector division.
We *don't* have `toInt_udiv`, since the only truly general statement we
can make does no better than unfolding the definition, and it's not
uncontroversially clear how to unfold `toInt` (see
`toInt_eq_msb_cond`/`toInt_eq_toNat_cond`/`toInt_eq_toNat_bmod` for a
few options currently provided). Instead, we do have `toInt_udiv_of_msb`
that's able to provide a more meaningful rewrite given an extra
side-condition (that `x.msb = false`).

This PR also upstreams a minor `Nat` theorem (`Nat.div_le_div_left`)
needed for the above from Mathlib.

---------

Co-authored-by: Kim Morrison <[email protected]>
luisacicolini pushed a commit to opencompl/lean4 that referenced this pull request Jan 21, 2025
This PR adds a `toFin` and `msb` lemma for unsigned bitvector modulus.
Similar to leanprover#6402, we don't provide a general `toInt_umod` lemmas, but
instead choose to provide more specialized rewrites, with extra
side-conditions.

---------

Co-authored-by: Kim Morrison <[email protected]>
JovanGerb pushed a commit to JovanGerb/lean4 that referenced this pull request Jan 21, 2025
This PR adds a `toFin` and `msb` lemma for unsigned bitvector division.
We *don't* have `toInt_udiv`, since the only truly general statement we
can make does no better than unfolding the definition, and it's not
uncontroversially clear how to unfold `toInt` (see
`toInt_eq_msb_cond`/`toInt_eq_toNat_cond`/`toInt_eq_toNat_bmod` for a
few options currently provided). Instead, we do have `toInt_udiv_of_msb`
that's able to provide a more meaningful rewrite given an extra
side-condition (that `x.msb = false`).

This PR also upstreams a minor `Nat` theorem (`Nat.div_le_div_left`)
needed for the above from Mathlib.

---------

Co-authored-by: Kim Morrison <[email protected]>
JovanGerb pushed a commit to JovanGerb/lean4 that referenced this pull request Jan 21, 2025
This PR adds a `toFin` and `msb` lemma for unsigned bitvector modulus.
Similar to leanprover#6402, we don't provide a general `toInt_umod` lemmas, but
instead choose to provide more specialized rewrites, with extra
side-conditions.

---------

Co-authored-by: Kim Morrison <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan changelog-library Library toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants