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

Add bitvector fallbacks to more nat ops in the simulator #1708

Merged
merged 5 commits into from
Jul 25, 2022

Conversation

m-yac
Copy link
Contributor

@m-yac m-yac commented Jul 18, 2022

Currently, the primitives for subNat, equalNat, and ltNat convert their arguments to bitvectors and perform the corresponding bitvector operation if their arguments are not both concrete Naturals.

This PR adds similar bitvector fallbacks to all the remaining natural number operations except expNat (because there is no corresponding bitvector exp function at the moment) and natCase. To do this we generalize the behavior of subNat, equalNat, and ltNat into the functions binNatOp and binNatToNatOp.

Fixes #1558

@m-yac m-yac added the subsystem: saw-core Issues related to the saw-core representation or the saw-core subsystem label Jul 18, 2022
@robdockins
Copy link
Contributor

As far as I can tell from just looking at the implementation, this seems like a good improvement.

This is a pretty tricky area of the codebase; if we can find a way to add some tests for these behaviors, that would be great.

@m-yac
Copy link
Contributor Author

m-yac commented Jul 19, 2022

Good idea! Looks like we haven't actually been running the saw-core test suite in the CI since we mirated the repo (yikes!), so I got that typechecking again, added it to the CI, and added two tests for each of the primitives I changed. I actually found a bug in my implementation of succOp while making the tests, so I'm very glad you made the suggestion!

@robdockins Let me know if this looks good now or if you think more tests are needed. I didn't cover every case for each operator, but since they're all implemented using unaryNatOp/binNatOp, the tests all together should have good coverage.

@RyanGlScott
Copy link
Contributor

Thanks for fixing the saw-core test suite, @m-yac! Make sure to put "fixes #1558" in the commit message so that we can finally close that one :)

@m-yac m-yac merged commit 178f9ed into master Jul 25, 2022
@m-yac m-yac deleted the sim-more-bv-nat-ops branch July 25, 2022 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
subsystem: saw-core Issues related to the saw-core representation or the saw-core subsystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test-sawcore fails to build
3 participants