Pharap / FixedPointsArduino

A fixed point arithmetic library for Arduino
Apache License 2.0
84 stars 9 forks source link

Replace shifts with enforced byte dropping in special cases #35

Open Pharap opened 6 years ago

Pharap commented 6 years ago

Aparently cases of >> 8, >> 16 and >>32 aren't being optimised to simply drop the bytes as I was originally expecting.

This isn't much of an issue for more powerful processors, but it's a bit of an issue for AVR chips.

The fix will be to manually enforce the byte dropping. I will look into the best way to do this.

If all else fails, type punning may be the only option.

Pharap commented 3 years ago

I started investigating this today to see if I could work out a way to make it viable.

Out of the techniques I've tried so far, the only one that both produced the correct result and resulted in a saving was:

template< unsigned Integer >
SFixed<Integer, 8> operator *(const SFixed<Integer, 8> & left, const SFixed<Integer, 8> & right)
{
    static_assert(((Integer + 1) * 2 + 8 * 2) <= FIXED_POINTS_DETAILS::BitSize<intmax_t>::Value, "Multiplication cannot be performed, the intermediary type would be too large");   

    using InternalType = typename SFixed<Integer, 8>::InternalType;
    using PrecisionType = typename SFixed<Integer * 2, 8 * 2>::InternalType;

    const auto intermediary = (static_cast<PrecisionType>(left.getInternal()) * static_cast<PrecisionType>(right.getInternal()));

    char buffer[sizeof(PrecisionType)];

    for(size_t index = 0; index < sizeof(PrecisionType); ++index)
        buffer[index] = reinterpret_cast<const char *>(&intermediary)[index];

    const InternalType result = *reinterpret_cast<const InternalType *>(&buffer[1]);

    return SFixed<Integer, 8>::fromInternal(result);
}

But this only appeared to result in an 8 byte saving in progmem, and I'm not 100% sure if it's 'legal'.

I may look into creating an AVR assembly implementation, and if that creates a significant enough saving then I'll consider adding it in, but most likely only when enabled using conditional compilation via an opt-in macro.

Regardless of which result I come up with, I won't be making this a general feature, it will be specifically for AVR because:

And I'd make it an opt-in feature because any solution is probably going to require the constexpr to be dropped because Arduino only targets C++11 and it's unlikely that the operations required to pull this off without creeping into the land of undefined behaviour will be usable under C++11's constexpr restrictions (which outlaw both variables and loops).