-
Notifications
You must be signed in to change notification settings - Fork 710
feat: add BitVec.cpop and lemmas
#11257
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
base: master
Are you sure you want to change the base?
Conversation
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.
Some early comments. This is going in a great direction.
BitVec.popCount and build theory around itBitVec.cpop and lemmas around it
|
Mathlib CI status (docs):
|
|
Reference manual CI status:
|
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 is going in a nice direction!
I think we should have a definition called cpopAux (x : BitVec w) : Nat := cpopAuxRec x w, and then define cpop in terms of cpopAux. In some sense cpopAux is the "real population count" (as a number), and cpop is a convenience function we provide to coerce the result into a bitvector.
Along similar lines, I would personally decouple the input and output widths of cpop, to have the signature be
def cpop {v : Nat} (w : Nat) (x : BitVec v) : BitVec w := BitVec.ofNat w xI'm not 100% sure this is the right design, but it feels a lot less arbitrary to me than choosing the output width, when popCount is really counting a natural number.
As always, this is not final, and I'm trying to navigate the design space just as much as you are, so I'm happy to pair on this!
|
Also, note that These tell us that popcount is a monoid homomorphism from bitvectors to naturals, and does the "correct" thing. I'd like us to prove these lemmas, and to define popcount such that these hold with small proofs :) So this would have us replace the |
0ad2adb to
05898c5
Compare
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.
Thank you for the update. A first set of comments.
It would also be nice if the default type conversion theorems are implemented, e.g., |
240c0e3 to
e9404ea
Compare
2245d6a to
ce3031a
Compare
2bae33e to
f8cd580
Compare
|
changelog-library |
BitVec.cpop and lemmas around itBitVec.cpop and lemmas
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 quite good! I'll defer to Markus after the two remaining remarks.
| omega | ||
| · omega | ||
|
|
||
| theorem cpop_cons {x : BitVec w} {b : Bool} : |
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.
Why are the toNat versions simp and not these ones?
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.
In the toNat theorem the rhs seems definitely easier than the lhs, for this theorem I don't think that's necessarily true, because of the setWidth. Do you think that is strictly easier even with the setWidth?
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.
AFAIU, the convention is to push toNat inwards. I do not think we have a canonical form for the plain cpop_cons.
This PR adds the definition of
BitVec.cpop, which relies on the more generalBitVec.cpopNatRec, and build some theory around it. The namecpopaligns with the RISCV ISA nomenclature.Co-authored-by: @tobiasgrosser, @bollu