-
Notifications
You must be signed in to change notification settings - Fork 53
Add IEEE-754 Floating Point to Bitvector Conversion Fallback #512
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: master
Are you sure you want to change the base?
Add IEEE-754 Floating Point to Bitvector Conversion Fallback #512
Conversation
…traint is not handed to the user
…gestion + add helper to get mantissa and exponent sizes for FPs (not finished for all solvers)
…n of user defined results for special FP numbers
…ll BV capable solvers
…ings and add it to JavaDoc
…BV transformation and width comparison
…umber mapping) and a test for FP size/mantissa/exponent in relation to BV width for toIeeeBitvector()
…ulaManager (based on solver return)
…d text for native toIeeeBitvector() in AbstractFloatingPointFormulaManager
@kfriedberger the SMTLIB2 standard defines the size of an FP as mantissa + exponent with the mantissa including the sign bit. Our current implementation excludes it, and we do not document this properly in all cases, especially the FP type constructor. This is problematic, as the standard defines the size of a BV representation of a FP as My proposal (already WIP):
I don't want to change the current FP-Type constructor, as users expect this to not change behavior. What do you think? I am happy about thoughts/ideas. Edit: it would probably be a good idea to also update the API for |
…tMantissaSizeWithSignBit() +
…he type/mantissa with/without sign bit and add sign bit to toString and toSMTLIBString representations
…a with/without sign bit and use those as far as possible to make the code unambiguous for mantissas
…IEEE BV method availability
…n bit and add some assertions for mantissa/exponent
…guous about the sign bit
…h new mantissa API
…heck against in FP toIeeeBitvector()
… the re-mapping of special values
Do we want to document the typical returns of solvers for special FP numbers? There are multiple well defined return values for NaN as bitvector for example. |
I mostly finished this as proposed. You can take a look. I feel like the API changes reduce the magic |
…gnBitImpl() to reduce ambiguity + improve name of variable
I now split all changes in regard to FP mantissa into a dedicated PR here. We should first handle that PR, merge it, and then continue here. |
…) instead of new implementation
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 am unsure whether this special handling for FP2IEEEBV is really required. This feature looks errorprone and quite complex to understand. I would prefer that solvers provide their internal implementation for this, and JavaSMT just provides the wrapper/bindings instead of such parts. There might be potential, but also the burden for the developer, especially when using a method that returns additional constraints.
*/ | ||
public static FloatingPointType getFloatingPointTypeWithSignBit( | ||
int exponentSize, int mantissaSizeWithSignBit) { | ||
return new FloatingPointType(exponentSize, mantissaSizeWithSignBit); |
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.
Where is the difference in this method to the one before? Both methods create the same object with the same parameters, even if they are named differently.
* Returns the size of the mantissa (also called a coefficient or significant), excluding the sign | ||
* bit. | ||
*/ | ||
// TODO: mark as soon to be deprecated |
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.
Do we really want to remove this simple method? Is the documentation not sufficient?
Please provide a replacement code snippet for inlining deprecated code.
+ "methods toIeeeBitvector" | ||
+ "(FloatingPointFormula, String) and/or " | ||
+ "toIeeeBitvector(FloatingPointFormula, String, Map)"); | ||
} |
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.
This message is quite misleading: The message will be seen by a user of the application, not by the developer calling JavaSMT. How will the user of the application (e.g., CPAchecker) change that call? That might be impossible.
@@ -519,4 +621,28 @@ protected static String getBvRepresentation(BigInteger integer, int size) { | |||
} | |||
return String.copyValueOf(values); | |||
} | |||
|
|||
public static final class BitvectorFormulaAndBooleanFormula { |
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.
Why not just use a Google-Auto-class? In the future this might be a record.
} | ||
|
||
@Override | ||
public BitvectorFormulaAndBooleanFormula toIeeeBitvector( |
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.
This method is weird. Does is really handle NaN as expected?
int getMantissaSizeWithSignBit(FloatingPointFormula number); | ||
|
||
/** Returns the size of the exponent for the given {@link FloatingPointFormula}. */ | ||
int getExponentSize(FloatingPointFormula number); |
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.
Do we really need these two methods?
The user already can access the FormulaType directly and extract exponentSize and mantissaSize from the returned FP-FormulaType.
protected int getExponentSizeImpl(Term fp) { | ||
return fp.sort().fp_exp_size(); | ||
} | ||
|
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 doupt the requirement for these two methods.
This PR adds 2 fallback methods for
FloatingPointManager.toIeeeBitvector(FloatingPointFormula)
:FloatingPointManager.toIeeeBitvector(FloatingPointFormula, String)
, returning the BV representation as a new variable with the given name. Special FP numbers are returned as the solver sees fit.FloatingPointManager.toIeeeBitvector(FloatingPointFormula, String, Map<FloatingPointFormula, BitvectorFormula>)
, returning the BV representation as a new variable with the given name. Special FP numbers can be mapped to expected results, with the default behavior (if no mapping has been added) being like the previous method.Both return a tuple
BitvectorFormulaAndBooleanFormula
, representing the bitvector of the input FP number, as well as the additional constraint needed asBooleanFormula
. The constraint needs to be added to all assertion stacks that make use of the returnedBitvectorFormula
.While implementing i found a problem in our implementation of
FloatingPointType
. Our returned significant precision is off by one. E.g. for single precision FP, we return significant size23
. But the standard defines exponent and significant as:Hence it should be
24
.As a consequence, we also return this wrongly for
toSMTLIBString()
.The solvers generally expect this to be the case, hence why we had 4 solvers with the code
type.getMantissaSize() + 1
, and only one solver without the+ 1
Also, we don't communicate any information about our interpretation of FP precisions, hence the users might think that by building
FormulaType.getFloatingPointType(8, 24)
(first input is exponent, second is significant) they get a single precision FP-type, but in reality they get a total size of 33 bits currently. Our static implementation of the single/double precisions is fine however.I added the capability to retrieve the precision from the solver directly, instead of using the JavaSMT type, as this was needed for this implementation.
This addition is currently hidden from users, but could be exposed if needed.I exposed this API publicly, as it seems to fit well withgetSize()
for bitvectors, and offers a type-independent, solver based way of getting the FP sizes.