Rockbox

What does this fix?
You changed the macros to truncate the multiplicands before doing the multiply which will just lose precision.

the FRACMUL function's operands are int32 only, but if you write like this:FRACMUL(0x80000000, aaa), the 0x80000000's sign bit will be treated as data value, then it becomes positive int64, instead of negative

the problem is that 0x80000000 is unsigned so it isn't sign extended when cast to long long.

FRACMUL((int32_t)0x80000000, aaa)

or

int32_t x = 0x80000000;
FRACMUL(x, aaa)

should work fine but indeed your patch fixes this, i'm just unsure if it's the right way or if your code using the macro shoul ensure the correct type.

if you use the code FRACMUL(0x80000000, aaa) in arm/cf target, the asm code works without any problem, i just make the c macro works like they do

Isn't 0x80000000 undefined anyway, because it doesn't fit into a signed interger?

NVM: C99 6.4.4.1 says if it doesn't fit an signed int, it'll be extended to unsigned int (for hex-numbers).

In this case, the code shouldn't make it signed (especially not long, since that doesn't fi). We need to deal with the fact that 0x80000000 is unsigned, or just write INT_MIN instead.

yes it is, but in int32 and int64, the 0x80000000 make different sense

the arm/cf's asm code and native c macro do different works when dealing with immediate int32 value smaller than 0xffffffff, one should be corrected, and i think the asm code do the right thing

I'm not sure I agree. The ASM should be a faster, but semantically equivalent, variant of the C code. I think the C code is not wrong, I'd say.

what's the real purpose of FRACMUL? Multiply two S.31 fractional integers and return the sign bit and the * 31 most significant bits of the result.
as described in the file itself, it should tread all the input operand as int32 by default, not int64, we use (long long) to convert the operand because we need to make the ”*“ return the 64bit value

this is a trap of macro define, it just replace the a with b and don't do a type check, if we write a real FRACMUL function, it should define like this: long FRACMUL(long x, long y), the x and y operands will always be treated as int32 and thus produce the correct result

I'd be in favour of turning the macros into inline functions as i find them easier to read and use.
Your problem is still that you are giving it an *unsigned * value and it's just a question of which code is responsible for the type being correct, btw casting to long is wrong anyway since long == long long on lp64 platforms like linux on amd64

I'd cast int32_t to int64_t on one operand, cast other to int32_t. DSP data has definite, platform-indepedent size (or should). These macros are supposed to take 32-bit arguments and return 32-bit results.

FRACMUL(x, y) ((int32_t)((int64_t)(int32_t)(x) * (int32_t)(y) >> 31))

I think I prefer inline functions too.

seems inline function in head file only not working in win32's simulator build?

i should add "if define" like below to prevent compilation error

#ifdef SIMULATOR
int32_t FRACMUL(int32_t x, int32_t y) {
#else
inline int32_t FRACMUL(int32_t x, int32_t y) {
#endif

Here's a patch that converts the FRACMUL macros to inlines (i left the name uppercase for now)

edit: there's a typo, in32_t instead of int32_t also, doesn't work for coldfire because of an intermediate constraint on the z param...

patch that works for coldfire builds too, needed to force inlining

Does this have any impact on codec performance?

Edit: Only the spc codec uses this during fading. So, no impact on codecs.

compiled ok, with no addition mem and no speed loss, will test the native code soon

small tweak to the coldfire FRACMUL_SHL to make it work even if gcc doesn't inline it but throw in a always_inline attribute anyway.

+1 from me

This change broke the build for the Nokia N8xx:

apps/fracmul.h: In function `dsp_set_crossfeed_cross_params':
apps/fracmul.h:79: warning: asm operand 4 probably doesn't match constraints
apps/fracmul.h:79: warning: asm operand 5 probably doesn't match constraints
apps/fracmul.h:79: warning: asm operand 4 probably doesn't match constraints
apps/fracmul.h:79: warning: asm operand 5 probably doesn't match constraints
apps/fracmul.h:79: error: impossible constraint in `asm'
apps/fracmul.h:79: error: impossible constraint in `asm'
apps/fracmul.h:78: warning: 't' might be used uninitialized in this function
apps/fracmul.h:78: warning: 't' might be used uninitialized in this function

According to betrik, the same is true for the "zipit Z2".

Reverting to the old macro makes it compile again.
FRACTMUL() compiles fine, FRACTMUL_SHL() doesn't.

Any idea what migth be going on?

Edit: gcc version: sbox-arm-linux-gcc (GCC) 3.4.4 (release) (CodeSourcery ARM 2005q3-2)

Is this fixed with r30094?

The issue is still valid, r30094 was just syntactic sugar.

this should fix it

Throwing in a FORCE_INLINE would probably fix it too

Thanks Nils, the patch did the trick. Please commit.