Skip to content

Correct C FixedMul() off-by-one errors

Tasos Sahanidis requested to merge (removed):fixedmul-c-fix into master

The FixedMul() C implementation would produce off by one results, causing constant desyncs on 64 bit builds and builds without an ASM implementation of the function.

This is fixed by shifting instead of dividing, possibly avoiding rounding errors.

The following example program can be used to confirm this issue.

Please note, the right shift in FixedMulWorking below results in undefined behaviour. A better implementation would be

return (fixed_t)(((UINT64)((INT64)a * b)) >> FRACBITS);
#include <stdint.h>
#include <stdio.h>

#define FRACBITS 16
#define FRACUNIT (1<<FRACBITS)

typedef int32_t INT32;
typedef int64_t INT64;
typedef INT32 fixed_t;

fixed_t FixedMul(fixed_t a, fixed_t b)
{
	return (fixed_t)((((INT64)a * b) ) / FRACUNIT);
}

fixed_t FixedMulWorking(fixed_t a, fixed_t b)
{
	return (fixed_t)((((INT64)a * b) ) >> FRACBITS);
}

fixed_t FixedMulAsm(fixed_t a, fixed_t b) // asm
{
    fixed_t ret;
    asm
	(
		 "imull %2;"           // a*b
		 "shrdl %3,%%edx,%0;"  // shift logical right FRACBITS bits
		:"=a" (ret)            // eax is always the result and the first operand (%0,%1)
		:"0" (a), "r" (b)      // and %2 is what we use imull on with what in %1
		, "I" (FRACBITS)       // %3 holds FRACBITS (normally 16)
		:"%cc", "%edx"         // edx and condition codes clobbered
	);
	return ret;
}

int main()
{
    fixed_t a = 16384;
    fixed_t b = -49261;
    printf("FixedMul: %i\n", FixedMul(a, b));
    printf("FixedMulWorking: %i\n", FixedMulWorking(a, b));
    printf("FixedMulAsm: %i\n", FixedMulAsm(a, b));
    return 0;
}

Which returns:

FixedMul: -12315
FixedMulWorking: -12316
FixedMulAsm: -12316

Can also be reproduced with many other numbers, some of them being shown here: e26c04c3ba05

Edited by Tasos Sahanidis

Merge request reports