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

[Merged by Bors] - fix: Data/Bitvec change definition of ofInt to the ring homomorphism #4525

Closed
wants to merge 3 commits into from

Conversation

ChrisHughes24
Copy link
Member

@ChrisHughes24 ChrisHughes24 commented May 31, 2023


After discussion with colleagues, we agreed that this was probably the more usual definition of ofInt.

Open in Gitpod

@ChrisHughes24 ChrisHughes24 added the awaiting-review The author would like community review of the PR label May 31, 2023
@semorrison
Copy link
Contributor

Sorry this has been sitting here unreviewed. I can't really tell why this change is being made. Perhaps you could incorporate more of that discussion into comments?

(But please, other reviewers, don't let my ignorance get in the way if you're inclined to merge.)

@ChrisHughes24 ChrisHughes24 changed the title refactor: Data/Bitvec change definition of ofInt to the ring homomorphism fix: Data/Bitvec change definition of ofInt to the ring homomorphism Jun 7, 2023
@tobiasgrosser
Copy link
Collaborator

Thank you @semorrison. As part of our effort to model compiler IRs as they arise in LLVM and MLIR, @bollu, @goens, and @ChrisHughes24 started to model bit-vector arithmetic. Aiming to contribute to the lean community where sensible, we want to explore in which way our requirements are compatible with mathlib. Truncating to the least significant bits (instead of preserving the sign but) is what programming languages typically do and also matches our own modeling efforts for LLVM/MLIR as well as the lean-smt people. We once again discussed this internally and feel this is a genuine correction to mathlib and the old code just required this correction.

Some CS details: in our work we obtained peephole rewrite test cases from the Alive project that are generic over the bitwidth and contain constants such as 1. Even though the value 1 is a common constant in many test cases, it is not in the set of values represented by a 1-bit vector when interpreted as 2's complement, which is just {0, -1}. Hence, we need to convert the bitwidth-generic value 1 into a 1-bit value. The new implementation that this PR introduces coincides with the conversion LLVM/MLIR chose for the very reason that it is more convenient for mathematicians, as it increases consistency across different bitwidths.

@digama0
Copy link
Member

digama0 commented Jun 8, 2023

The new definition is definitely more sensible than the old one.

bors r+

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review The author would like community review of the PR labels Jun 8, 2023
bors bot pushed a commit that referenced this pull request Jun 8, 2023
#4525)

Co-authored-by: Chris Hughes <33847686+ChrisHughes24@users.noreply.github.com>
@bors
Copy link

bors bot commented Jun 8, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title fix: Data/Bitvec change definition of ofInt to the ring homomorphism [Merged by Bors] - fix: Data/Bitvec change definition of ofInt to the ring homomorphism Jun 8, 2023
@bors bors bot closed this Jun 8, 2023
@bors bors bot deleted the Bitvec-ofInt branch June 8, 2023 08:28
@tobiasgrosser
Copy link
Collaborator

Thank you @digama0

qawbecrdtey pushed a commit to qawbecrdtey/greedoid-mathlib4 that referenced this pull request Jun 12, 2023
leanprover-community#4525)

Co-authored-by: Chris Hughes <33847686+ChrisHughes24@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants