[FFmpeg-cvslog] r18936 - in trunk/libavcodec: Makefile mlpdsp.c

[Ramiro Polla]

Reimar D?ffinger wrote:
> On Mon, May 25, 2009 at 04:42:28PM -0300, Ramiro Polla wrote:
>> I still don't really understand what llvm is complaining about, but is 
>> there a way to make llvm stop complaining without altering the object 
>> code in gcc? There should be a way to correctly specify this...
>>
>> Besides, how bad can a cast to the same type be?
> 
> Sorry, I missed that readval is "r" for 64 bit.

I forgot to include that in my message...

> In that case, I think that's a bug in LLVM, it should ignore nop-casts
> (the types are exactly the same).
> Alternatively, you could change the READVAL macro to include the cast.
> Though I think it would really be a good idea to go over all those
> constraints and check them, because if not plain wrong they are at least
> silly, e.g.
> 
>> @@ -166,7 +167,7 @@
>>            /* 2*/"+r"(sample_buffer),
>>            /* 3*/RDWRVAL(blocksize)
>>          :
>> -          /* 4*/READVAL((x86_reg)mask),
>> +          /* 4*/READVAL(mask_reg),
>>            /* 5*/READVAL(firjump),
>>            /* 6*/READVAL(iirjump),
> 
> Why force firjump and iirjump into a register ("r") instead of allowing
> both register and reference ("g")?

Because I assume it will be slower to read the value from reference. I 
tried to force as much as I could into registers to make the code 
faster. Using "g" lead gcc to randomly use reference on x86_64 and run 
out of registers for x86_32, so I decided to force both "r" and "m" when 
I thought it was best. This is my first attempt at gcc inline asm and 
any kind of asm optimization, so I certainly can have chosen a 
suboptimal (or even downright stupid =) setting.

>> #if ARCH_X86_64
>>         , /* 8*/"r"((int64_t)coeff[0])
>>         , /* 9*/"r"((int64_t)coeff[1])
>>         , /*10*/"r"((int64_t)coeff[2])
>> #endif /* ARCH_X86_64 */
> 
> Why isn't READVAL used, at least for consistency?

Because these should specifically be "r", and READVAL could have been 
something else, but it isn't...

> Why int64_t and not x86_reg?

Good point. I think both this and the previous point could be changed, 
but I don't have much of a strong opinion about them.

> Wouldn't it maybe have been more readable to put all the constraints
> that differ under #if ARCH_X86_64 instead of having a RDWRVAL, READVAL

> and COUNTER macro _and_ still needing the if (I admit it is a bit of
> code duplication...)

That's what I tried to avoid. Maybe I obfuscated too much to avoid 
duplication... I'll send a patch for this later tonight.

Ramiro Polla