Defined constant that express the number of bits for exponent in single and double precision. Added bit masks values and other properties for quad precision floating point numbers that specifically targets architectures defined in PlatfromDefs.h. The exponentWidth values were added to be used in LongDoubleBitsX86.h where the implementation to set the exponent component uses this and the bitWidth value. The need occurred because of the 80-bit quad precision implementation.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
@sivachandra why is the mantissa/exponent bits info duplicated between FloatProperties.h and FPBits.h in separate structs?
Comment Actions
Typically all changes that add functionality should have tests. For example, a simple one where we construct some representation and mess around with the bits to make sure they work should be good enough.
@sivachandra are there existing tests for this sort of stuff?
Also, could you explain why you're adding the number of bits for exponents in the description? Something along the lines of not being able to use the sizeof the BitsType due to 80 long double vs 128 bit int.
| libc/utils/FPUtil/FloatProperties.h | ||
|---|---|---|
| 65 | no need to change it here since it matches the existing code, but it'd be nice to use using BitsType = uint64_t over a typedef, using is more modern and easier to read | |
| 102 | No need to change here, but it'd be nice if we didn't have to have constants like mantissaMask, which is derivable from mantissaWidth, in each FloatProperties. Rather, we could have the user of mantissaMask calculate the mask itself, e.g. ((BitsType)1 << T::mantissaWidth) - 1 Or something like template <typename T>
struct FloatPropertiesExt {
static constexpr T::BitsType mantissaMask = ((BitsType)1 << T::mantissaWidth) - 1;
};and use FloatPropertiesExt<T>::mantissaMask where you'd previously use FloatProperties<T>::mantissaMask. | |
| 139–140 | does this actually work? I thought long double was 80 bits on Linux/x86 what is the point of FloatType anyway? it's going to be ambiguous in the cases where we have long double == double since uint64_t will map to both | |
| libc/utils/FPUtil/FloatProperties.h | ||
|---|---|---|
| 35 | The quiet NAN masks are probably incorrect. What is a quiet NAN and what is a signalling NAN depends on the target architecture. I will remove them in a separate patch. In the meanwhile, do not add them to the new structs you are defining. | |
| 63 | A high level comment: These #if defined clauses should be mutually exclusive. As in, the next one in the series should be #elif defined, and the last one #else. | |
| 87 | As pointed out above, do not add this member. | |
| 109 | As pointed out above, do not add this member. | |
| 125 | Here and elsewhere in this patch, a more C++ way to do the compile time cast/conversion is to do it this way: BitsType(...) << 64 | BitsType(...) As opposed to the C style casts you have. The above style is probably also more readable. | |
| 126 | These should also go. So, do not add the new class below. | |
| 135 | As pointed out above, do not add this member. | |
| 138 | As pointed out above, do not add this new class. | |
Comment Actions
Once we moved to packed structs, we considered FloatProperties.h to be deprecated. There are only a handful of files where FloatProperties.h is included and so we were in the process of removing it. But, now that we want to resurrect this header file, we should scrub out the listings in FPBits.h and replace all uses with values defined in FloatProperties.h.
The way I see it, this is the first patch in a series of at least two patches. As in, @hedingarcia should also prepare the follow up patch(es) which remove(s) the packed struct in FPBits.h and do all other related cleanups and updates. So, the changes in this patch will get tested in the follow up patch(es). This patch is essentially capturing specs so there is nothing to test.
| libc/utils/FPUtil/FloatProperties.h | ||
|---|---|---|
| 139–140 | While this class should go away, I want to point out something about @aeubanks' comment. When we say 80 bits, we are referring to the encoding size and not the storage size. So, while long double values are encoded to an 80-bit representation, their storage is 128 bits on Linux/x86_64 and 96 bits on Linux/i386. The additional bits in the storage are garbage padding. | |
Comment Actions
I also want to add a note for @lntue who might be missing the context of this change: @hedingarcia is working on bringing up LLVM libc on Windows. He found that packed structs do not reliably work on Windows. The idea is to switch away from packed structs and use hand constructed methods in FPBits to access the encoding. With that as a goal, this patch is adding the encoding properties for long double numbers.
| libc/utils/FPUtil/FloatProperties.h | ||
|---|---|---|
| 102 | should this constant be ULL instead of U? | |
| 104 | Will this mask also get garbage padding bits above 80 bits? If it is the case then you might need to get the exact constant, something like signMask - BitType(1) - mantissaMask | |
| 125 | Should these constants be ULL instead of U? Same for the line below. | |
Comment Actions
[libc] Fix the patch alongside with the recommendations given by reviewers.
Took out the bit mask values for quiet NAN in the long double FloatProperties implementations (left the quiet NAN value in double since it was already there). Made the macros statement exclusive where only one implementation for long double can be used depending on the architecture of the machine. Changed the cast for a more C++ style. Eliminated the struct FloatType for __uint128_t.
Certain bit mask values like mantissaMask where left as literal values and were not derived from mantissaWidth to keep the general convention used before I made the patch.
Comment Actions
[libc] Fix of the patch, redefining bit mask so they become easier to maintain
Use width integer values to specify bit masks in order to maintain a compact value and not depend
on long inline literals. The value of exponentWidth became more helpful since signMask uses it in
all the single, double, and quad implementation. A fix was made for the exponentMask in the 80-bit
quad precision struct since the previous mask was not taking care of the extra bits used for padding.
Comment Actions
Almost there. Thanks for your patience.
| libc/utils/FPUtil/FloatProperties.h | ||
|---|---|---|
| 81 | The correct terminology is exponentBias. Since you are touching this, can you replace exponentOffset with exponentBias at all places in this file? | |
| 98 | See below about the implicit bit. So, this should include another + 1; | |
| 101 | The 80-bit encoding has another quirk - the implicit "1" bit is explicit. It is at bit position 64 (ie. the 65th LS bit). See this for more information: https://en.wikipedia.org/wiki/Extended_precision. Or, another different way to do the same would be: static constexpr BitsType exponentMask = ((BitsType(1) << exponentWidth) - 1) << (mantissaWidth + 1); | |
| libc/utils/FPUtil/FloatProperties.h | ||
|---|---|---|
| 94 | Ah! I missed that you made this 64 in which case some of my comments below are incorrect. I think our algorithms expect this to be 63 as listed here: https://github.com/llvm/llvm-project/blob/main/libc/utils/FPUtil/LongDoubleBitsX86.h#L20. | |
| 101 | If mantissaWidth is 64, I think what you have is correct. But, if mantissaWidth is 63, you will have to do the additional adjustment I pointed out above. A correction to my comment above: the implicit bit is the 64th least significant bit and not the 65th least significant bit. | |
Comment Actions
[libc] Fix of the patch, redefining mantissaWidth for the 80-bit long double and other properties
Renamed the variable names of exponentOffset to exponentBias for is the correct terminology.
Changed the mantissaWidth value to 63 in the 80-bit long double implementation and the
corresponding masks for sign and exponent. Added also an explicitBit boolean in all single,
double, and quad precision. The purpose of this is to maintain a general template struct that
works for all architectures and the value of explicitBit is intended to be used in FPBits.h for shifting the exponent component. Only the 80-bit long double will require that explicitBit to be 1 but for the rest it will be 0 and will only use the mantissaWidth in order to shift the exponent.
Comment Actions
[libc] Fix of the patch, eliminated the explicitBit member
The explicitBit variable lost its purpose since the value of the bitWidth in all the float,
double, and long double structs can be use to shift the exponent component of the floating
point number even for 80-bit long double. No need for it to be kept as a float property.
That is why the bitWidth value was changed in 80-bit from 128 to 80, this actually represents
now the encoding width.
The quiet NAN masks are probably incorrect. What is a quiet NAN and what is a signalling NAN depends on the target architecture. I will remove them in a separate patch. In the meanwhile, do not add them to the new structs you are defining.