change pretty-printing of floats because of LLVM IR restrictions #137
Conversation
As was pointed out by @tomsmeding here: GaloisInc#135 (comment) we cannot use 32-bit (8 characters) "float" constants in LLVM IR. Instead, we must embed the 32-bit float as 64-bit double constants with the same meaning.
|
The particular case that triggered the problem for me is fixed with this PR, thanks! But this is only really a spot test, because this only accidentally popped up in the test suite of a larger codebase. Thanks for the very quick PR! |
| if isInfinite f || isNaN f | ||
| then text "0x" <> text (showHex (castFloatToWord32 f) "") | ||
| else float f | ||
| -- WARNING: You should **not** use `castFloatToWord32` or `float` here. LLVM IR does not | ||
| -- support 32-bit floating point constants, instead it wants to see 32-bit compatible 64-bit | ||
| -- constants. We want to preserve the exact mantissa (zero-extended to the right from 23 to | ||
| -- 52 bits), which happens to be the behavior of `float2Double`. | ||
| text "0x" <> text (showHex (castDoubleToWord64 (float2Double f)) "") |
There was a problem hiding this comment.
After this PR, we will now always display float-typed constants in hexadecimal form, even values that aren't infinite or NaNs. Is that desirable? (As #135 (comment) shows, it's still possible to write things like float 1.0.)
Regardless of what choice we make here, we should add some test cases that exercise the pretty-printing code for non-infinite, non-NaN values at type float and double.
There was a problem hiding this comment.
Something that I apparently haven't run into yet, but as also mentioned in the documentation, LLVM will only accept "readable" constants if they exactly match a floating-point value. In particular, 1.3 is accepted for double but not for float.
It may simply be easier to always output floating-point constants in hexadecimal form, so that we never run into issues like this. (And for my particular purpose, I don't care very much about readability of floating-point constants.)
There was a problem hiding this comment.
Wow. That's quite tricky.
In that case, I agree that if there isn't a straightforward way to determine if LLVM will accept the output of calling the float function, then we should conservatively print all float constants in hexadecimal form. It would also be worth mentioning the exact floating-point value requirement in the comments here, as I wasn't aware of that additional subtlety until just now.
There was a problem hiding this comment.
Ah, if float prints things more nicely, and in a way that LLVM accepts, I can look into making this change less radical.
|
In a discussion with the group, we feel like we may just want to always print the hexadecimal version, possibly providing a flag to add a best-effort decimal expansion as a comment nearby where applicable. Setting this as a draft until I have more time to handle these considerations. |
As was pointed out by @tomsmeding here:
#135 (comment) we cannot use 32-bit (8 characters) "float" constants in LLVM IR.
Instead, we must embed the 32-bit float as 64-bit double constants with the same meaning.