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

FStar.BV: expose bvshl, bvshr and bvmod with bit vector rhs #3116

Merged
merged 6 commits into from
Dec 22, 2023

Conversation

amosr
Copy link
Contributor

@amosr amosr commented Nov 29, 2023

The bit-shifting and modulus operations in FStar.BV currently take natural numbers in the right-hand side. This incurs some conversion overhead, as the SMT-LIB primitives take bit vectors. This commit implements bit vector native versions called bvshl', bvshr' and bvmod_unsafe.

@mtzguido
Copy link
Member

NB still testing - just wanted to check if ci worked

No problem with using PRs to check this, but make ci from a clean repo is pretty much exactly what CI does (and it may also be faster depending on your setup). Make docker-ci goes a step further and runs the check within a container, like the CI machine does, but unless you're changing external dependencies make ci is usually good enough.

@amosr
Copy link
Contributor Author

amosr commented Nov 30, 2023

Thanks Guido, make ci is very helpful

@amosr amosr changed the title WIP FStar.BV: expose bvshl, bvshr and bvmod with bit vector rhs FStar.BV: expose bvshl, bvshr and bvmod with bit vector rhs Nov 30, 2023
@amosr amosr marked this pull request as ready for review November 30, 2023 06:36
@amosr
Copy link
Contributor Author

amosr commented Nov 30, 2023

Sorry, I marked it as "ready to review", but it's not actually encoding the new bvshl' operator correctly yet.

@amosr amosr marked this pull request as draft November 30, 2023 07:01
@amosr amosr marked this pull request as ready for review December 1, 2023 05:04
amosr added 3 commits December 1, 2023 16:04
The bit-shifting and modulus operations in FStar.BV currently take natural numbers
in the right-hand side. This incurs some conversion overhead, as the SMT-LIB primitives
take bit vectors. This commit implements bit vector native versions called `bvshl'`,
`bvshr'` and `bvmod_unsafe`.
The SMT encoding for bvdiv_unsafe was wrapping the bit vector argument
in a boxed BV and then unwrapping as an int. I have fixed this and added
a test.
@amosr
Copy link
Contributor Author

amosr commented Dec 1, 2023

I think this is ready for review now. I'll ping @nikswamy too since we discussed this on Slack earlier.

I've added new binary functions that take two bit vectors: bvshl', bvshr', bvmul'. For modulo, I've named the new function bvmod_unsafe to match the existing bvdiv_unsafe. (I believe the encoding for bvdiv_unsafe was also slightly incorrect, which I've fixed.)

I am not convinced about the names bvshl' etc. It's not very descriptive and it's unfortunate that it's inconsistent with bvdiv_unsafe. But I can't think of anything short and descriptive, so I've just used them for now. Happy to change it.

(I was also not sure where to put the test. It's not really a bug report, but I couldn't see a better place for it.)

@nikswamy nikswamy merged commit dbe43a1 into FStarLang:master Dec 22, 2023
0 of 2 checks passed
@nikswamy
Copy link
Collaborator

Many thanks Amos! I just made a couple of small fixes and merged this. I also checked that this didn't cause any regressions in the related projects in the Everest repo.

As a next step, it might be worth considering a breaking change that swaps the names around to make the primed version of the operators the default ... but that's for another day.

@amosr
Copy link
Contributor Author

amosr commented Dec 23, 2023

Thanks Nik!

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