Add fixed point math library #316
Conversation
| template<uint8_t ISize, uint8_t FSize, bool Signed = true> class Value; | ||
|
|
||
| template<uint8_t ISize, uint8_t FSize, bool Signed> | ||
| class Value { |
There was a problem hiding this comment.
fixedpoint::Value is a bit of a mouthful, and using namespace fixpoint; would leave just Value, which is to ambiguous to use.
Instead, I'd recommend placing the class and the typedefs in the global namespace, and using namespace fixedpoint_literals for the literals. using namespace fixedpoint_literals; isn't too much typing, since it's once per file, and it would allow referring to arbitrary types as FixedPoint<12,34> x;.
| // Size of the underlying storage for the different views of the data. | ||
| // The total combined storage will be used for representing the actual value. | ||
| using IntType = std::conditional_t<Signed, | ||
| signed _BitInt(((ISize + 7) / 8) * 8), |
There was a problem hiding this comment.
Extract the widths out into named constexpr constants; the overall logic here gets hard to read out of all the arithmetic.
| class Value { | ||
| protected: | ||
|
|
||
| // Used to calculate the bitmasks for both the integer part and fractional part |
There was a problem hiding this comment.
Can you provide more detail about the semantics of onecount and the return value?
|
|
||
| // Size of the underlying storage for the different views of the data. | ||
| // The total combined storage will be used for representing the actual value. | ||
| using IntType = std::conditional_t<Signed, |
There was a problem hiding this comment.
Since they're used in public methods, they should be public, since it's otherwise impossible to name them without auto.
|
|
||
| /// Copy constructor for a fixed point value with different sizes of Int/Frac | ||
| template<uint8_t OI, uint8_t OF, bool S> | ||
| [[clang::always_inline]] constexpr Value(const Value<OI, OF, S>& o) { |
There was a problem hiding this comment.
Similar to the above, this should be implicit if OI >= ISize && OF >= FSize (i.e. lossless), and explicit otherwise (lossy). I think some std::enable_if-fu can express this; may take some experimentation.
| using FP_8_8 = FP_S8_8; | ||
| using FP_12_4 = FP_S12_4; | ||
|
|
||
| [[clang::always_inline]] constexpr FP_S8_8 operator ""_s8_8(long double fixed) { |
There was a problem hiding this comment.
Add a preprocessor check for C++20.
| SetF(f); | ||
| } | ||
| /// Constructor to convert floating point values into fixed point | ||
| [[clang::always_inline]] constexpr Value(long double f) { |
There was a problem hiding this comment.
This case is trickier; converting a long double to a fixed point number may or may not be lossy. It'll almost always be lossy though. And at runtime, it will be quite slow.
If we have C++20 support (preprocessor check), it seems like these constructors should be consteval to ensure that they can only be evaluated at compile time. (I think consteval constructors work this way; if it doesn't , NAR.) In either case, we'd probably want non-constexpr versions of this for the three float types.
| } | ||
| /// Constructor to convert floating point values into fixed point | ||
| [[clang::always_inline]] constexpr Value(long double f) { | ||
| Set(f * (1 << FSize)); |
There was a problem hiding this comment.
// Multiplying by a power of two only increases the exponent of the floating point number, which is exact so long as overflow does not occur. Rounding a floating point number to whole is always exact.
| SetI(i); | ||
| } | ||
|
|
||
| /// Copy constructor for a fixed point value with different sizes of Int/Frac |
There was a problem hiding this comment.
The various lossy conversion functions below need descriptions of what inputs are exactly representable, and if not exactly representable, either a description of the behavior, or a description of which input and output combinations produce undefined behavior.
There's a choice point here; truncation of fractional bits should definitely be legal, but it could either round to even like floating point or truncate like integers. Given that this is intended to be an integer-based approximation to floating point for highest speed, I'd assume the latter, but fixed point libraries I've seen in the wild do the former. In either case, that behavior should be documented.
| // Direct value accessor and setter methods | ||
|
|
||
| /// Returns just the integral portion | ||
| [[clang::always_inline]] constexpr IntType AsI() const { return (val & INT_MASK) >> FSize; } |
There was a problem hiding this comment.
We've been using snake_case for variables and functions; so as_i() etc.
| [[clang::always_inline]] constexpr StorageType Get() const { return val; } | ||
| /// Update just the integral portion | ||
| [[clang::always_inline]] constexpr void SetI(IntType v) { | ||
| val = (val & ~INT_MASK) | static_cast<StorageType>((v) & (INT_MASK >> FSize)) << FSize; |
There was a problem hiding this comment.
This kind of logic seems a lot easier to express using bit fields in a struct; does that also produce sufficient codegen quality?
| return *this; | ||
| } | ||
|
|
||
| [[clang::always_inline]] constexpr Value operator +(const Value& o) const { |
There was a problem hiding this comment.
Similarly to the above, seems most sensible to take o by value, not by constant reference. Here and throughout.
| } | ||
|
|
||
| [[clang::always_inline]] constexpr Value operator +(const Value& o) const { | ||
| Value<ISize, FSize> n = val; |
There was a problem hiding this comment.
This doesn't seem right; there isn't a constructor that takes in storage type.
Maybe Value n = *this; instead? Here and throughout.
| return *this; | ||
| } | ||
|
|
||
| template<typename T, typename = typename std::enable_if<std::is_arithmetic<T>::value, T>::type> |
There was a problem hiding this comment.
The enable_if is superfluous here and below; the template will already only expand for types that we can divide the value by. Those are exactly the types that it's meaningful to implement division for.
| return *this; | ||
| } | ||
|
|
||
| [[clang::always_inline]] Value &operator++() { |
There was a problem hiding this comment.
Hmm, I'd think that this operation would increment the integer part, not the base representation. It should be logically equivalent to += 1, and right now it isn't.
It might be okay to have a break here if there's a compelling reason, but that reason should definitely be written down somewhere at the very least.
| } | ||
|
|
||
| [[clang::always_inline]] constexpr friend bool operator==(const Value& a, IntType b) noexcept { | ||
| return a.val == b; |
There was a problem hiding this comment.
These aren't correct; they don't mask away the fractional bits or shift the integer to put it in line with the integer bits.
|
There's a great deal of relevant prior art here. Examples: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0037r7.html
Have courage... I don't believe that you are solving an easy problem. |
I'd like to push back on this, at least for multiplication of two fixed points, since implementing it well is quite nontrivial. I don't think a minimum viable version of this needs to support it, since it's also not very common in the NES scene that is requesting the feature (correct me if I'm wrong on this one). |
I think it's worth considering now, exactly because of its nontriviality. It is exactly the sort of thing that's hard to add later, once you bet the farm on a specific architecture. At the very least, I think it's not unreasonable to ask @jroweboy to review the references that I provided, and report back and see if he's had any novel enlightenment. Even if we don't implement it now, I'd like to be assured that we can implement it later. |
It's not hard to add pretty much anything later; we still have very loose backwards compatibility guarantees, and the newer the feature, the looser they are. The only exception is things that a bunch of other features in the SDK end up depending on, but that's wildly unlikely for a C++ fixed point library. Better is the enemy of good, and once a few problems have been resolved, this will be pretty useful even at its current scope. |
|
The NES community has waited approximately 39 years for this feature; I don't necessarily think a day or two of delay will break anyone's heart. I'm not saying don't do it; I'm just saying that now is the best time for due diligence and stealing good ideas. |
I don't think that literature reviews should be a prerequisite for contributing to a hobby compiler that we work on for fun. llvm-mos should welcome authors with all degrees of seriousness, and the purpose of asking more from someone in a code review should only to be ensure that the fun the author is having is balanced well with the fun the users of what they write will have. In any case, I don't think this PR is the correct venue for this discussion; we should move this to Discord. In the interim, I'll use my sway as current maintainer to say that this PR should proceed as-is if and while we have a broader discussion about what we want our bar to be. |
Thanks to feedback here, I'm testing out using the bitfield support built-in to the compiler to see if the code gen is still just as good. Note that even in this original code it still supports 5.3 as the
Yes! And there's also prior art for high level languages with the 6502 as well. I spoke with pubby (maker of nesfab) which is a compiler with builtin fixed point support to get some ideas of what is expected from this feature. NESFab only support byte sized fractional values, but still has compile time float -> fixed support like I want to offer here. Multiplication extends the values into the combined size of the output type (so if you have an 8.8 * 12.4 it becomes 20.12) and division is unimplemented as its rather troublesome for something people don't use in practice.
I mentioned this in the original post, but the test suite will be added to llvm-mos-lit test cases once I've established the implementation a little better. I'm down to work towards a multiplication solution that addresses all of your concerns, especially since there's existing art for this on 6502 systems (well at least the one i'm familiar with), but I'm not so keen on division as it quickly becomes problematic for very little benefit IMO. |
So long as the fixed point type sizes are identical, |
Address many of the review comments as well
…romotion and also add multiplication
- Like C integers, silently truncate on implicit conversion to a smaller type. - Compound assignments convert to the join type above then truncate, as C integers do.
Add a general purpose fixed point math library. Leaving it as a draft PR until I get the time to add the test cases to llvm-test-suite and fix any issues we find there. (I wasn't able to build locally so I've been developing in godbolt in the meantime)
Also I think there's some ambiguous resolve errors for comparing fixed point numbers right now that I need to fix before merging too. I would expect that
number = 9.4_8_8; number > 7.7_8_8should work, but it can't decide which comparison operator to use and errors out currently.I am just pushing this here so I don't lose track of it, but if you want to do an early review to help out with code clarity/style/etc feel free to do so!