-
Notifications
You must be signed in to change notification settings - Fork 47
Move complete handling of functions to MathLibraryModel, support additional functions
#750
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: dev
Are you sure you want to change the base?
Conversation
("The CNF conversion does not support quantifiers")
(precise except for NaN)
…and fmod explicitly
|
@schuessf @bahnwaerter does this lead to conflicts with the refactored memory model? |
I don't think so, this PR basically just changes |
# Conflicts: # trunk/source/CACSL2BoogieTranslator/src/de/uni_freiburg/informatik/ultimate/cdt/translation/implementation/base/expressiontranslation/BitvectorTranslation.java
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.
👍 Thanks for making this effort to further improve our library handling, and even extending it!
I don't know much about floating-point arithmetic, so I couldn't review those aspects in detail. I have left some comments/questions mostly on the architecture.
| } | ||
|
|
||
| private ExpressionResult handleSqrt(final IDispatcher main, final IASTFunctionCallExpression node, | ||
| final ILocation loc, final String name, final CPrimitive type) { |
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.
minor refactoring suggestion: why not let these kinds of methods take a CPrimitives and have one new CPrimitive(...) call in the method, rather than one in every usage? This will improve readability of getFunctionModels.
...ltimate/cdt/translation/implementation/base/expressiontranslation/ExpressionTranslation.java
Outdated
Show resolved
Hide resolved
...ltimate/cdt/translation/implementation/base/expressiontranslation/ExpressionTranslation.java
Outdated
Show resolved
Hide resolved
...ltimate/cdt/translation/implementation/base/expressiontranslation/ExpressionTranslation.java
Outdated
Show resolved
Hide resolved
...ltimate/cdt/translation/implementation/base/expressiontranslation/ExpressionTranslation.java
Outdated
Show resolved
Hide resolved
...k/ultimate/cdt/translation/implementation/base/expressiontranslation/IntegerTranslation.java
Outdated
Show resolved
Hide resolved
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.
Just a couple more small comments.
...cdt/translation/implementation/base/expressiontranslation/BitvectorFloatingPointHandler.java
Show resolved
Hide resolved
...cdt/translation/implementation/base/expressiontranslation/BitvectorFloatingPointHandler.java
Show resolved
Hide resolved
...ultimate/cdt/translation/implementation/base/expressiontranslation/BitvectorTranslation.java
Outdated
Show resolved
Hide resolved
...ltimate/cdt/translation/implementation/base/expressiontranslation/ExpressionTranslation.java
Outdated
Show resolved
Hide resolved
...cdt/translation/implementation/base/expressiontranslation/BitvectorFloatingPointHandler.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public Expression getCurrentRoundingMode() { |
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.
Another candidate that could potentially be moved to IFloatingPointHandler
...t/translation/implementation/base/expressiontranslation/UnsupportedFloatingPointHandler.java
Show resolved
Hide resolved
...t/translation/implementation/base/expressiontranslation/UnsupportedFloatingPointHandler.java
Show resolved
Hide resolved
You're right. The changes only affect the memory model, which particularly affects the memory structure and addressing. There may be a small merge conflict, but this should be very easy to resolve. I would suggest that we merge the memory model first (as soon as the last unsoundness issues have been resolved) and then merge this pull request. |
trunk/examples/programs/FloatingPoint/regression/c/all/ctrans-float-copysign-2.c
Outdated
Show resolved
Hide resolved
...t/translation/implementation/base/expressiontranslation/UnsupportedFloatingPointHandler.java
Outdated
Show resolved
Hide resolved
| // signbit(x) := isNegative(x) ? 1 : 0; | ||
| new OverapproxVariable("sign of NaN", loc).annotate(nondet); | ||
| final Expression zero = mExpressionTranslation.constructLiteralForIntegerType(loc, intType, BigInteger.ZERO); | ||
| builder.addStatement(StatementFactory.constructIfStatement(loc, mFloatHandler.isNan(loc, argument, type), |
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.
@danieldietsch @Heizmann As you have worked with floats in the past, does this handling of signbit (incl. the overapproximation) seem reasonable?
| type))); | ||
| // TODO: Overapproximate if second is NaN | ||
| new OverapproxVariable("sign of NaN", loc).annotate(nondet); | ||
| builder.addStatement(StatementFactory.constructIfStatement(loc, mFloatHandler.isNan(loc, first, type), |
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.
@danieldietsch @Heizmann As you have worked with floats in the past, does this handling of copysign (incl. the overapproximation) seem reasonable?
| final List<ExpressionResult> arguments = handleFloatArguments(main, node, loc, name, 2, type); | ||
| final Expression first = arguments.get(0).getLrValue().getValue(); | ||
| final Expression second = arguments.get(1).getLrValue().getValue(); | ||
| return new ExpressionResultBuilder().addAllExceptLrValue(arguments) |
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.
@danieldietsch In 042d866 you removed also the support for remainder, claiming in this comment that our handling was also unsound. In this PR, however, I added support again using the SMT function fp.rem. The documentations for fp.rem and remainder seemed to match and I could not find any unsoundness. Do you remember the issue with remainder?
PR #714 refactored the way standard functions in C are modeled in Boogie. As a result, separate classes are now used to handle functions from specific C libraries. In particular, the class
MathLibraryModelis responsible for modeling functions frommath.h. However, in case ofMathLibraryModelthe actual translation is delegated toBitvectorTranslation::constructOtherUnaryFloatOperationandBitvectorTranslation::constructOtherBinaryFloatOperation(the other implementations of those methods inIntegerTranslationsimply throw an exception, since these float operations are not supported), whileFloatFunctionserves as some kind of connection (with some hacky string operations to determine the C type). Consequently, whenever support for new math library functions is added, three classes had to be changed (MathLibraryModel,BitvectorTranslation,FloatFunction), which made the process cumbersome.Therefore, this PR removed all the handling of standard functions from
BitvectorTranslation, as well as the (rather hacky) classFloatFunctionand moved the translation of functions frommath.hentirely toMathLibraryModel(similar as done in other library models). To facilitate this, new methods were added toExpressionTranslationto serve as a proper API. InBitvectorTranslationthese methods implement calls to SMT-defined floating-point theory functions, while inIntegerTranslation, they throw exceptions.Additionally, this PR added support for more functions from
math.h(with the help of overapproximations):signbit,remainder,copysign: They were removed in 042d866 (see this comment), and since then only an exception was thrown. This was due to an unsoundness, since we were not able to handle negative NaNs in SMT, but they can occur in C (with the help ofcopysign). While some efforts were made to handle these cases precisely (see Represent ambivalent floats as bitvectors #440), they were left incomplete, so this PR uses overapproximations instead. Specifically, when NaN values appear as arguments tocopysignorsignbit, the result is safely overapproximated, which remains precise enough for most of the cases.sin,exp, ...) were added, with an error handling (i.e., ±0, ±∞ and NaN as arguments) according to the C standard. For all other return values, a very coarse overapproximation is used, capturing simple properties - for examplesin(x)is always between -1 and 1, andexp(x)is always at leastx+1. Despite this coarse overapproximation, the translation should still be precise enough to verify some (very simple) tasks of the neural-networks benchmarks.