Redefined FPBits.h and LongDoubleBitsX86 so its implementation works for the Windows
and Linux platform while maintaining a packed memory alignment of the precision floating
point numbers. For its size in memory to be the same as the data type of the float point number.
This change was necessary because the previous attribute((packed)) specification in the struct was not working
for Windows like it was for Linux and consequently static_asserts in the FPBits.h file were failing.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Fixed error signaled when running the tests for LdExpTest.h by ensuring that the value passed to set mantissa
did not lead to overload to the exponent value in FPBits.h and LongDoubleBitsX86.h.
Added asserts to detect those unintentional overflows in setMantissa() and setExponent().
the title should say something about creating a custom packed int, the getter and setter part is a small detail that shouldn't be in the title
libc/utils/FPUtil/FPBits.h | ||
---|---|---|
21–22 | since we're basically just forwarding MantissaWidth to FloatProperties::mantissaWidth, we can delete all the specializations below and do template <typename T> struct MantissaWidth { static constexpr unsigned value = FloatProperties<T>::mantissaWidth; }; Or even better, delete these and and only use FloatProperties::mantissaWidth. But maybe that can be done in a follow up patch. | |
45 | I think we don't need the __llvm_libc::fputil:: since we're arleady inside that namespace | |
45–78 | we should replace all uses of integer with encoding.valueFP | |
48 | I don't really like valueFP, how about something like bits | |
54 | these comments should probably be deleted, they're not super useful | |
79 | this is very confusing, can we just use mantissaWidth? | |
91 | bool? |
[libc] Fix of the patch, refactored FPBits.h to have template structs
Made one template struct for MantissaWidth, one for ExponentWidth, and another for FPUIntType.
Removed namespaced calls, comments and changed variable name valueFP to bits in FPBits.h and LongDoubleBitsX86.h.
Changed the return data type of getSign() from bool to uint8_t.
the commit message is a bit verbose:
- no need to say that the patch was fixed
- no need to say that you updated all getters/setters of exponent/mantissa/sign, that is a direct and obvious consequence of FPBits work
- no need to say that we're using FloatProperties.h, that's obvious if you look at the patch
- the ultimate reason for this patch isn't that static_asserts are failing, it's that attribute((packed)) isn't portable and doesn't work on Windows
in general the commit message should be more of a "why" rather than a "what are the steps I took to create this patch"; try to figure out exactly what a reader of this patch needs to know and don't over-explain details since that clutters the message
if something tricky is being done then that should be explained of course, but most of this patch is self-explanatory if you know the reasoning behind the change
libc/utils/FPUtil/FPBits.h | ||
---|---|---|
56 | @sivachandra is this the right way to assert? | |
83 | not sure if this should be a bool or a uint8_t... |
libc/utils/FPUtil/FPBits.h | ||
---|---|---|
79 | The reason why we cannot use mantissaWidth is because in the 80-bit long double implementation the value for that field is 63 instead of 64. An explicit bit is considered in the 63 bit as the integer part of the significand (https://en.wikipedia.org/wiki/Extended_precision ). This exception leads to that long expression in getExponent(), because with only the value of mantissaWidth the shift will still require to move one bit in order to return all the bits of the exponent. |
libc/utils/FPUtil/FPBits.h | ||
---|---|---|
79 | I thought that only applied to the version in LongDoubleBitsX86 |
libc/utils/FPUtil/FPBits.h | ||
---|---|---|
79 | Yes, however should the same implementation in LongDoubleBitsX86.h be kept the same even in FPBits.h? |
[libc] Fixing shift in getExponent() and setExponent()
Refactoring the previous shift to retrieve and set the exponent
with the mantissaWidth value in FPBits.h
libc/utils/FPUtil/FPBits.h | ||
---|---|---|
79 | I see what you mean now, yes since LongDoubleBitsX86.h takes care of the long double implementation, this file only should take in consideration that explicit bit. FPBits.h does not have to work around this case since it is used for single and double. Thank you for catching that detail. |
Sorry for the delay in the review. I have gone through the history and it mostly looks good. I have a few comments about the cosmetics.
libc/utils/FPUtil/FPBits.h | ||
---|---|---|
29–30 | Can this struct be removed now? | |
44–78 | I would actually prefer if we got rid of encoding completely [1]. Adding the new methods to FPBits directly would reduce the verbosity when calling them. The integer value is currently called integer, but as @aeubanks suggests elsewhere, bits is more appropriate may be. However, to reduce the churn, you can choose to keep it as integer or bits depending on whats convenient to you. [1] - We had a separate encoding field because it was essentially that when we could use bit-fields. Moreover, it helped us distinguish it with the other fields. With that gone, we can choose to remove it and eliminate that "complexity" and verbosity. | |
45 | For consistency, I would name UIntType as BitsType here as well. But, you can choose to add a TODO here and do that "cleanup" in a follow up change. | |
53 | Why are these methods required? Probably because you got rid of the integer field of FPBits? If you add the rest of the methods to FPBits directly (see below), this should not be required as the integer / bits value can be directly set or read. | |
56 | Not sure what line this pertains to. But, after this change, the only assert that would be required is something like: assert(sizeof(T) == sizeof(UIntVal), ...); | |
69 | You should probably name this method unbiasedExponent to distinguish it from the real exponent value returned by getExponent(). | |
83 | To which line was this comment originally for? |
libc/utils/FPUtil/FPBits.h | ||
---|---|---|
29–30 | I'd say clean this, along with Exponent/MantissaWidth up in a later patch, there are lots of uses of these | |
56 | oh, I think the code was deleted for some reason I spent a while debugging a failure Hedin was running into. Turns out sometimes when setting the mantissa/exponent, it was actually larger than the max value (IIRC at the very bottom of NormalFloat.h). With the struct bitfield it automatically took care of that, but here we needed to mask out the extra bits. The question is whether to do that in the setter or do that in the caller. If we do that in the setter we preserve the existing behavior. But I'm slightly in favor of forcing callers to handle that and adding an assert in the setter that the bits don't overflow. This is probably a bit faster since we don't have to do the bitmask on every setter, and may catch future issues. The question about the assert was, is assert(cond); the right way to assert? As in in debug modes it'll run the assert and crash, and in release modes the assert won't be there for perf reasons. Is <assert.h> the right thing to include? | |
83 | this was for getSign() |
libc/utils/FPUtil/FPBits.h | ||
---|---|---|
56 | Ah, OK! I should have said static_assert in my comment. The general rule we follow is:
Some of the above are checked by the libc lint rules implemented as part of clang-tidy. They are only run on the full build builders: https://lab.llvm.org/buildbot/#/workers/120 So, we cannot include assert.h or use assert. Whether the value should be checked in the caller or the setter, there are a few algorithms which assume the setters are doing it (there is a deliberate intention to overflow the mantissa.) So, I would say do the masking in the setter to retain functionality. In practice, it isn't a big runtime penalty as the setting usually happens at the very end of a complex algorithm. |
[libc] Removed encoding/FPUIntType struct and renamed getExponent()
Eliminated encoding struct and moved its data and member fields into FPBits.
Removed the FPUIntType struct since it was not being referenced.
Logically, it doesn't matter. Are there benefits wrt compiler driven optimizations? If yes, then suggest the one which should be preferred?
Otherwise, LGTM as well. Thanks for patiently resolving the comments.
libc/utils/FPUtil/FPBits.h | ||
---|---|---|
76 | Sorry for late comments! Since you already updated to get* and set*, can you also add getBits and setBits? I think they would be a better than the current uintval() (and technically no equivalent of setBits). | |
libc/utils/FPUtil/NextAfterLongDoubleX86.h | ||
53 | I'm a bit curious about generated assembly of this line. @sivachandra : can we check if there is any regression with this one? I think no regression for O2 and O3 would be good enough. |
libc/utils/FPUtil/FPBits.h | ||
---|---|---|
76 | These get/setBits members were used previously with a different name but now they are not required anymore since bits value can be directly set or read from FPBits. | |
libc/utils/FPUtil/NextAfterLongDoubleX86.h | ||
53 | The object files generated before and after this patch are the same, at least after running the check there was no difference between the files at O2 optimization level. |
libc/utils/FPUtil/FPBits.h | ||
---|---|---|
49 | I'm rather surprised that this change was necessary. When working with a specific bit layout, using a unsigned integer type and bitwise operations can be a good idea--I don't think the language standards give many guarantees about how the bits are allocated (e.g., MSB first or LSB first?). But I'm not sure why the compiler (we are talking about clang here, right?) didn't pack the structures the same way on all platforms. Perhaps when targeting Windows, clang is trying too hard to be compatible with MSVC. With MSVC, when defining bit fields, they will pack by default--but only if the underlying types are all the same. Since the old types were UIntType,, uint16_t, and uint8_t--all of which have different alignment requirements--MSVC doesn't pack those. But if they had all been defined as uint32_t, MSVC would certainly have done the right thing. And thus, maybe clang would have as well. |
since we're basically just forwarding MantissaWidth to FloatProperties::mantissaWidth, we can delete all the specializations below and do
Or even better, delete these and and only use FloatProperties::mantissaWidth. But maybe that can be done in a follow up patch.