-
Notifications
You must be signed in to change notification settings - Fork 92
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
Use optimized overflow operation everywhere #2405
Conversation
This is related to the performance degradation we are seeing in the toolchain upgrade: model-checking#2293
It looks like the performance is not always better... :( |
I do wonder if this proof is also somewhat unstable. I noticed that in my other PR (#2407) that had minimum changes to the model, there was also a performance degradation of 47% for the same test. |
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.
I'd like to ask for an explanation as to why the no-longer-failing checks are ok.
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.
- Revert shift checks expectations that were removed. - Improve comments
That's fair! My bad, I should've added an explanation before. I added it as part of an answer to one of your question and addressed the issue in the call-out section. In a nutshell, the arithmetic check that was being added by CBMC was redundant. |
Description of changes:
This is related to the performance degradation we are seeing in the toolchain upgrade: #2293
Resolved issues:
Towards #1483. There are a few remaining instances of add_overflow and mul_overflow in intrinsics.rs.
Related RFC:
Optional #ISSUE-NUMBER.
Call-outs:
This PR has a side effect of removing redundant checks inserted by CBMC for checked arithmetic operations.
Testing:
How is this change tested? Existing tests and manual tests with my other branch where I'm doing the upgrade
Is this a refactor change? yes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.