-
Notifications
You must be signed in to change notification settings - Fork 715
feat: add leading zero counter BitVec.clz and bitblaster circuit/infrastructure
#8546
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
Conversation
|
Mathlib CI status (docs):
|
src/Std/Tactic/BVDecide/Bitblast/BVExpr/Circuit/Lemmas/Operations/Clz.lean
Outdated
Show resolved
Hide resolved
src/Std/Tactic/BVDecide/Bitblast/BVExpr/Circuit/Lemmas/Operations/Clz.lean
Outdated
Show resolved
Hide resolved
src/Std/Tactic/BVDecide/Bitblast/BVExpr/Circuit/Lemmas/Operations/Clz.lean
Outdated
Show resolved
Hide resolved
|
https://en.wikipedia.org/wiki/Find_first_set states:
You may want to use this as a justification in the commit message of why you implement See also https://clang.llvm.org/docs/LanguageExtensions.html#intrinsics-support-within-constant-expressions. |
src/Std/Tactic/BVDecide/Bitblast/BVExpr/Circuit/Lemmas/Operations/Clz.lean
Show resolved
Hide resolved
tobiasgrosser
left a comment
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.
This looks great to me. I have a couple of minor nit-picking comments, but am otherwise happy with this.
For the PR title/body:
- add a
.at the end of your PR/commit message - make sure
BitVecis mentioned in the PR title - the PR adds clz as a new operation to
BitVec/Basic.lean. It would be good if this is mentioned and justified. E.g., "This PR adds a new operationBitVec.clztogether with bitlasting support viabv_decide. `Count leading zeros (clz) is ... We add a specialized bitblaster which requires only AIG nodes linear in the number of bits of the original bitvector expressions and consequently should be easier to bitblast than just a rewrite to existing bitvector expressions.
src/Std/Tactic/BVDecide/Bitblast/BVExpr/Circuit/Lemmas/Operations/Clz.lean
Outdated
Show resolved
Hide resolved
src/Std/Tactic/BVDecide/Bitblast/BVExpr/Circuit/Lemmas/Operations/Clz.lean
Outdated
Show resolved
Hide resolved
|
changelog-library |
BitVec.clz and bitblaster infrastructure BitVec.clz and bitblaster circuit/infrastructure
hargoniX
left a comment
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.
Looks generally like a fine approach!
I did not comment on the clz BitVec proofs yet as I suspect making the definition tail recursive might cause these proofs to turn out quite differently.
I also wonder whether we should add seval simprocs for BitVec.clz right away as well.
src/Std/Tactic/BVDecide/Bitblast/BVExpr/Circuit/Impl/Operations/Clz.lean
Outdated
Show resolved
Hide resolved
src/Std/Tactic/BVDecide/Bitblast/BVExpr/Circuit/Lemmas/Operations/Clz.lean
Outdated
Show resolved
Hide resolved
|
thanks @hargoniX for the review! To have a tail-recursive function, I reused |
This PR adds a new
BitVec.clzoperation and a correspondingclzcircuit tobv_decide, allowing to bitblast the count leading zeroes operation. The AIG circuit is linear in the number of bits of the original expression, making the bitblasting convenient wrt. rewriting.clzis common in numerous compiler intrinsics (see here) and architectures (see here).Co-authored by @bollu.