Rockbox

Tasklist

FS#12120 - fix c implement of FRACMUL & FRACMUL_SHL

Attached to Project: Rockbox
Opened by JoshuaChang (JoshuaChang) - Tuesday, 17 May 2011, 06:53 GMT
Last edited by Nils Wallménius (nls) - Thursday, 30 June 2011, 08:33 GMT
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.8.1
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

this is a small patch to fix the c language implement of FRACMUL & FRACMUL_SHL
the original ones dealing the immediate int32 negative value as int64 positive value.
This task depends upon

Closed by  Nils Wallménius (nls)
Thursday, 30 June 2011, 08:33 GMT
Reason for closing:  Fixed
Additional comments about closing:  fix comitted
Comment by Nils Wallménius (nls) - Tuesday, 17 May 2011, 07:06 GMT
What does this fix?
You changed the macros to truncate the multiplicands before doing the multiply which will just lose precision.
Comment by JoshuaChang (JoshuaChang) - Tuesday, 17 May 2011, 07:15 GMT
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
Comment by Nils Wallménius (nls) - Tuesday, 17 May 2011, 07:48 GMT
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.

Comment by JoshuaChang (JoshuaChang) - Tuesday, 17 May 2011, 08:00 GMT
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
Comment by Thomas Martitz (kugel.) - Tuesday, 17 May 2011, 12:32 GMT
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.
Comment by JoshuaChang (JoshuaChang) - Tuesday, 17 May 2011, 13:44 GMT
yes it is, but in int32 and int64, the 0x80000000 make different sense
Comment by JoshuaChang (JoshuaChang) - Tuesday, 17 May 2011, 13:47 GMT
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
Comment by Thomas Martitz (kugel.) - Tuesday, 17 May 2011, 13:52 GMT
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.
Comment by JoshuaChang (JoshuaChang) - Tuesday, 17 May 2011, 14:07 GMT
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
Comment by JoshuaChang (JoshuaChang) - Tuesday, 17 May 2011, 14:16 GMT
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
Comment by Nils Wallménius (nls) - Tuesday, 17 May 2011, 19:05 GMT
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
Comment by Michael Sevakis (MikeS) - Tuesday, 17 May 2011, 22:31 GMT
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.
Comment by JoshuaChang (JoshuaChang) - Wednesday, 18 May 2011, 03:21 GMT
seems inline function in head file only not working in win32's simulator build?
Comment by JoshuaChang (JoshuaChang) - Wednesday, 18 May 2011, 03:23 GMT
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
Comment by Nils Wallménius (nls) - Wednesday, 18 May 2011, 07:13 GMT
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...
Comment by Nils Wallménius (nls) - Wednesday, 18 May 2011, 16:09 GMT
patch that works for coldfire builds too, needed to force inlining
Comment by Andree Buschmann (Buschel) - Wednesday, 18 May 2011, 18:47 GMT
Does this have any impact on codec performance?

Edit: Only the spc codec uses this during fading. So, no impact on codecs.
Comment by JoshuaChang (JoshuaChang) - Thursday, 19 May 2011, 05:37 GMT
compiled ok, with no addition mem and no speed loss, will test the native code soon
Comment by Nils Wallménius (nls) - Thursday, 19 May 2011, 07:02 GMT
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.
Comment by Andree Buschmann (Buschel) - Thursday, 19 May 2011, 18:46 GMT
+1 from me
Comment by Thomas Jarosch (thomasjfox) - Tuesday, 28 June 2011, 21:09 GMT
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)
Comment by Andree Buschmann (Buschel) - Wednesday, 29 June 2011, 06:05 GMT
Is this fixed with r30094?
Comment by Thomas Jarosch (thomasjfox) - Wednesday, 29 June 2011, 09:07 GMT
The issue is still valid, r30094 was just syntactic sugar.
Comment by Nils Wallménius (nls) - Wednesday, 29 June 2011, 09:58 GMT
this should fix it
Comment by Nils Wallménius (nls) - Wednesday, 29 June 2011, 10:03 GMT
Throwing in a FORCE_INLINE would probably fix it too
Comment by Thomas Jarosch (thomasjfox) - Wednesday, 29 June 2011, 16:59 GMT
Thanks Nils, the patch did the trick. Please commit.

Loading...