[FFmpeg-devel] 回复： patch 1/4: libavcodec/ppc/pixblockdsp.c: fixget_pixels_altivec() and diff_pixels_altivec() for POWER LE

[Reimar Döffinger]

On 8 October 2014 04:39:20 CEST, rongyan <rongyan236 at foxmail.com> wrote:
>Reimar,
>Sorry for late response.
>'HAVE_VSX' is automatically enabled only when HAVE_ALTIVECT is enabled
>and the CPU is little endian. So if the altivec implementation is
>broken in little endian, the HAVE_VSX will not be enabled.‍

That will make it work normally.
But what if someone wants to disable VSX?
For example because they want to test the performance difference, or want to debug an error in some the VSX code or because they want to use some tool that does not yet support it, or because their toolchain does not support it?
They'd suddenly end up with code that is broken instead of just slower.
I strongly believe that CPU optimizations should be possible to disable without the code being broken.
Or the other way round, broken optimizations should first be disabled and only then fixed optimizations added, not both at once.
But since these issues already exist that probably should not block the patch, but I strongly suggest adding a --disable-vsx option and testing it, for x86 these kind of options have shown themselves quite important for debugging. 

>
>------------------ 原始邮件 ------------------
>发件人: "Reimar Döffinge";<Reimar.Doeffinger at gmx.de>;
>发送时间: 2014年10月1日(星期三) 凌晨1:51
>收件人: "FFmpeg development discussions and
>patches"<ffmpeg-devel at ffmpeg.org>; 
>
>主题: Re: [FFmpeg-devel] patch 1/4: libavcodec/ppc/pixblockdsp.c:
>fixget_pixels_altivec() and diff_pixels_altivec() for POWER LE
>
>
>
>On Tue, Sep 30, 2014 at 03:51:55PM +0800, rongyan wrote:
>> Hi,
>>  I present 4 patches to fix bugs for POWER8 little endian.
>>  I will send 4 patches in 4 different email. This is the first.
>> The fate test result after merge these 4 patches can be found on
>http://fate.ffmpeg.org/ by search "ibmcrl", also attached here to
>facilitate the review:
>> 
>> 
>> ​
>> The passed test cases increased from 1649/2169 to 1675/2174.
>
>This one seems wrong.
>I assume HAVE_VSX is a feature related define, which means it should
>be possible to disable it without breaking anything.
>If the altivec implementation is broken on little-endian,
>the correct bug-fix is to disable it.
>Then as a next step can a different optimized implementation be added.
>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel at ffmpeg.org
>http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>_______________________________________________
>ffmpeg-devel mailing list
>ffmpeg-devel at ffmpeg.org
>http://ffmpeg.org/mailman/listinfo/ffmpeg-devel